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