[PATCH] D62829: [clang-tidy] Check for dynamically initialized statics in headers.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:40
+    return;
+  Finder->addMatcher(varDecl().bind("var"), this);
+}
----------------
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()`.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:39
+    return;
+  Finder->addMatcher(varDecl(hasGlobalStorage()).bind("var"), this);
+}
----------------
Do you want to restrict this matcher to only variable declarations that have initializers, or are you also intending for this check to cover cases like:
```
// At file scope.
struct S { S(); } s;
```


================
Comment at: clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:55
+  SourceLocation Loc = Var->getLocation();
+  if (!Loc.isValid() || !utils::isPresumedLocInHeaderFile(Loc, *Result.SourceManager,
+                                                          HeaderFileExtensions))
----------------
We have an AST matcher for this (`isExpansionInSystemHeader()`).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829