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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.


================
Comment at: lib/AST/ASTImporter.cpp:2954
+    return Found->hasExternalFormalLinkage();
+  else if (Importer.GetFromTU(Found) == From->getTranslationUnitDecl()) {
+    if (From->isInAnonymousNamespace())
----------------
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.


================
Comment at: unittests/AST/ASTImporterTest.cpp:65
 // -fms-compatibility or -fdelayed-template-parsing.
-struct ParameterizedTestsFixture : ::testing::TestWithParam<ArgVector> {
+class CompilerOptionSpecificTest : public ::testing::Test {
+protected:
----------------
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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57232