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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits