[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Thank you for working on this!

I have two concerns:

- The logic when should we import the initializer of a VarDecl is duplicated. I think to have better maintainability it would be better to have only one implementation of the decision in a function and call it multiple times.
- While the static analyzer is currently the only user of CTU lib it might change in the future. Only importing initializers of const variables is an analyzer specific decision which might not be good for other potential users of the CTU library. Maybe it would be better to be able to configure the ClangExtDefMapGen somehow, so it is able to both export only vardecls that are required by the analyzer or anything?



================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:163
+static bool hasBodyOrInit(const VarDecl *D, const VarDecl *&DefD) {
+  return D->getAnyInitializer(DefD) != nullptr;
+}
----------------
The `!= nullptr` part is usually not written in LLVM codebase.


================
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:357
+          return true;
+        if (!RTy->hasConstFields())
+          return true;
----------------
I wonder what would happen with types that has const fields and user written constructors. In case we will not simulate the effect of the constructor and will not be able to set the const fields maybe we should exclude those as well?


================
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:362
+      // Only import if const.
+      if (!Ctx->getCanonicalType(VD->getType()).isConstQualified())
+        return true;
----------------
Maybe this check could be hoisted so we do not need to repeat in both branches?


================
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:369
+
+    if (VD->getAnyInitializer() != nullptr)
+      return true;
----------------
Redundant `!= nullptr`.


================
Comment at: tools/clang-extdef-mapping/ClangExtDefMapGen.cpp:68
+    QualType VTy = VD->getType();
+    bool containsConst = VTy.isConstQualified();
+    if (!containsConst && !VTy.isNull())
----------------
Variables should start with a capital letter.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421