[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer
Gabor Marton via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 14 07:54:55 PST 2018
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
More information about the cfe-commits
mailing list