[PATCH] D122805: [analyzer][ctu] Only import const and trivial VarDecls

[Gabor Marton via Phabricator via cfe-commits]

martong marked 3 inline comments as done.
martong added a comment.

In D122805#3419129 <https://reviews.llvm.org/D122805#3419129>, @steakhal wrote:

> Please add a test case where the class is trivial but the global variable of such is non-const, thus the initialized expression is not imported.

I've added a new case, but that is not very meaningful, because `clang-extdef-mapping` is not capable of emitting the USR for the non-const variable.



================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:112
 
-// Returns true if the variable or any field of a record variable is const.
-bool containsConst(const VarDecl *VD, const ASTContext &ACtx);
+/// Returns true if it makes sense to import a foreign variable declaration.
+/// For instance, we don't want to import variables that have non-trivial types
----------------
steakhal wrote:
> definition
thx, fixed.


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:195
+bool shouldImport(const VarDecl *VD, const ASTContext &ACtx) {
+
   CanQualType CT = ACtx.getCanonicalType(VD->getType());
----------------
steakhal wrote:
> extra blank line
Thanks!


================
Comment at: clang/test/Analysis/Inputs/ctu-other.cpp:128
 SCNest extSCN = {.scn = {.a = 9}};
-SCNest::SCN extSubSCN = {.a = 1};
+extern SCNest::SCN const extSubSCN = {.a = 1};
 struct SCC {
----------------
steakhal wrote:
> AFAIK you don't need the `extern` here, since **this is** the definition of that global, not only a declaration.
> Similarly applies to the rest of the cases.
Actually, linkage of `const` variable definitions are internal in C++,  unless the `extern` modifier is provided. So, we need it.

If it was a non-const then the definition's linkage would be external.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122805