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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Hi Rafael,

This is a great patch and good improvement, thank you! I had a few minor comments.
(Also, I was thinking about what else could we import in the future, maybe definitions of C++11 enum classes would be also worth to be handled by CTU.)



================
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:114
+  llvm::Expected<const VarDecl *>
+  getCrossTUDefinition(const VarDecl *VD, StringRef CrossTUDir,
+                       StringRef IndexName);
----------------
r.stahl wrote:
> xazax.hun wrote:
> > I wonder if we need these overloads. Maybe having only the templated version and having a static assert for the supported types is better? Asking the other reviewers as well.
> They are not required, but I thought they make the interface more clear and help prevent implementation in headers.
I consider the new overload just great. Later, if we want we still can extend the interface with a template which would call the overloaded functions.


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:117
+}
+static bool hasDefinition(const VarDecl *D, const VarDecl *&DefD) {
+  return D->getAnyInitializer(DefD) != nullptr;
----------------
The naming here is confusing for me, would be better to be `hasInit`, because there are cases when a VarDecl has an initializer but it is not a definition:
```
      struct A {
        static const int a = 1 + 2; // VarDecl: has init, but not a definition
      };
      const int A::a; // VarDecl: definition
```
In the above case we probably want to import the initializer and we don't care about the definition.


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:120
+}
+template <typename T> static bool hasDefinition(const T *D) {
+  const T *Unused;
----------------
`hasDefinitionOrInit` ?


================
Comment at: test/Analysis/Inputs/ctu-other.cpp:79
+};
+extern const S extS = {.a = 4};
----------------
Could we have another test case when `S::a` is static?


================
Comment at: test/Analysis/func-mapping-test.cpp:18
+struct S {
+  int a;
+};
----------------
Could we have one more test when we have a `static` member variable?


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

https://reviews.llvm.org/D46421