czhang marked 4 inline comments as done.
czhang added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:40
+ return;
+ Finder->addMatcher(varDecl().bind("var"), this);
+}
----------------
aaron.ballman wrote:
> czhang wrote:
> > lebedev.ri wrote:
> > > Most of the matching should be done here, not in `check()`.
> > I believe that there is no way to use the AST matching language to express
> > hasConstantDeclaration.
> You can write a local AST matcher to hoist this functionality into the
> `check()` call, though. Some of it is already exposed for you, like
> `hasInitializer()`, `isValueDependent()`, and `isConstexpr()`.
Perhaps not done in the most elegant way, but there is some amount of
non-trivial side effecting caused by checkInitIsICE that makes me a little wary
try to chain this more declaratively.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:55
+ SourceLocation Loc = Var->getLocation();
+ if (!Loc.isValid() || !utils::isPresumedLocInHeaderFile(Loc,
*Result.SourceManager,
+
HeaderFileExtensions))
----------------
aaron.ballman wrote:
> czhang wrote:
> > aaron.ballman wrote:
> > > We have an AST matcher for this (`isExpansionInSystemHeader()`).
> > Isn't this for system headers only, not just included 'user' headers?
> Ahh, good point! Still, this should be trivial to make a local AST matcher
> for.
Seems like many of the google tidy checks choose to relegate this header
checking (whence I copied this bit) in the checker, rather than match
registration time. Not sure what the pros and cons are of hoisting, but I left
it there to follow the convention of the other checks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62829/new/
https://reviews.llvm.org/D62829
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
