NoQ added inline comments.
================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2340
+ if (Node->doesThisDeclarationHaveABody())
+ checkUnsafeBufferUsage(Node);
+ return true;
----------------
This code will grow bigger when more warnings are added, and it's quite likely
that most warnings would want to duplicate themselves in each of the Visit
methods. Maybe let's make a single function from which all per-function
warnings are invoked?
Also, hmm, `UnsafeBufferUsageReporter` can probably be reused between callables.
Actually, here's an even fancier approach: maybe let's move the visitor outside
the function so that it didn't get in the way of readability, and run warnings
in a lambda so that we didn't need to explicitly capture anything:
```lang=c++
class CallableVisitor {
llvm::function_ref<void(const Decl *)> callback;
public:
CallableVisitor(llvm::function_ref<void(const Decl *)> callback):
callback(callback)
{}
VisitFunctionDecl(FunctionDecl *Node) {
if (Node->doesThisDeclarationHaveABody()) {
callback(Node);
}
}
// More Visit() methods.
};
void clang::sema::AnalysisBasedWarnings::IssueWarnings(const
TranslationUnitDecl *TU) {
// Common whole-TU safety checks go here.
if (S.hasUncompilableErrorOccurred() || Diags.getIgnoreAllWarnings())
return;
// Whole-TU variables for all warnings go here. Then they're implicitly
captured,
// so no need to list them in the constructor anymore.
UnsafeBufferUsageReporter UnsafeBufferUsageDiags(S);
const bool UnsafeBufferUsageEmitFixits =
Diags.getDiagnosticOptions().ShowFixits && S.getLangOpts().CPlusPlus20;
CallableVisitor([&](const Decl *Node) {
// Per-decl code for all warnings goes here.
if (!Diags.isIgnored(diag::warn_unsafe_buffer_operation, D->getBeginLoc())
||
!Diags.isIgnored(diag::warn_unsafe_buffer_variable, D->getBeginLoc())) {
checkUnsafeBufferUsage(Node, UnsafeBufferUsageDiags,
UnsafeBufferUsageEmitFixits);
}
}).TraverseTranslationUnitDecl(TU);
}
```
I think that's a pretty neat way to organize this. This way it's immediately
clear which code is executed how many times.
================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2369-2371
+ // Emit unsafe buffer usage warnings and fixits.
+ if (!Diags.isIgnored(diag::warn_unsafe_buffer_operation, SourceLocation()) ||
+ !Diags.isIgnored(diag::warn_unsafe_buffer_variable, SourceLocation())) {
----------------
Ooo interesting, there is indeed a good reason to keep those checks outside; we
shouldn't make an entire extra pass when none of the analysis-based warnings
are enabled.
I suggest making the comment a bit more generic, so that users felt empowered
to add more checks there and reuse the visitor for their warnings.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146342/new/
https://reviews.llvm.org/D146342
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
