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