[PATCH] D57232: [ASTImporter] Check visibility/linkage of functions and variables

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added inline comments.


================
Comment at: lib/AST/ASTImporter.cpp:2954
+    return Found->hasExternalFormalLinkage();
+  else if (Importer.GetFromTU(Found) == From->getTranslationUnitDecl()) {
+    if (From->isInAnonymousNamespace())
----------------
shafik wrote:
> We don't really need an `else ` here and it would be more like an early exit which is what llvm style guide suggests.
> 
> In the same vein I would do:
> 
>     if (Importer.GetFromTU(Found) != From->getTranslationUnitDecl())
>        return false; 
> 
> so a second early exit and remove a level of nesting at the same time.
It's a good catch, thanks.


================
Comment at: unittests/AST/ASTImporterTest.cpp:65
 // -fms-compatibility or -fdelayed-template-parsing.
-struct ParameterizedTestsFixture : ::testing::TestWithParam<ArgVector> {
+class CompilerOptionSpecificTest : public ::testing::Test {
+protected:
----------------
shafik wrote:
> Are these changes directly related to the visibility change? There is a lot of noise that is not obviously related to the description in the PR.
> 
> 
> If not maybe it should be a separate PR?
Actually, it will be more precise to create first a patch which contains the tests related refactor, I agree. I'll do that and this patch will be dependent upon that.


================
Comment at: unittests/AST/ASTImporterTest.cpp:2523
+  Decl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  cast<DeclContext>(ToTU)->dumpDeclContext();
+  ASSERT_EQ(DeclCounter<FunctionDecl>().match(ToTU, functionDecl(hasName("f"))),
----------------
balazske wrote:
> Is this dump needed? (The test should not write unnecessary text output. But debug statements can be leaved in the test, possibly in comment.)
Thanks for catching this, good point.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57232