[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I am happy to discuss and review a revised version of this change.


Repository:
  rC Clang

https://reviews.llvm.org/D44100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53697: [ASTImporter][Structural Eq] Check for isBeingDefined

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I reverted this commit since it caused a regression in the lldb test suite, 
specifically TestDataFormatterLibcxxVector.py.

See the logs here 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/12003/

Please monitor the lldb built bots when modifying ASTImporter.


Repository:
  rC Clang

https://reviews.llvm.org/D53697



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53697: [ASTImporter][Structural Eq] Check for isBeingDefined

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

@martong Hello, if I look at the specific test that was failing 
`TestDataFormatterLibcxxVector.py` it only has the following decorator 
`@add_test_categories(["libc++"])` so that should not prevent it from running 
on Linux if you are also building libc++. If you do a local cmake build and you 
build libc++ as well does it still skip the test?


Repository:
  rC Clang

https://reviews.llvm.org/D53697



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53702: [ASTImporter] Set redecl chain of functions before any other import

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Did you ever resolve the issue of libcxx tests not running 
https://reviews.llvm.org/D53697


Repository:
  rC Clang

https://reviews.llvm.org/D53702



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: include/clang/AST/Expr.h:1407
 public:
-  enum CharacterKind {
-Ascii,
-Wide,
-UTF8,
-UTF16,
-UTF32
-  };
+  enum CharacterKind { Ascii, Wide, UTF8, UTF16, UTF32 };
 

riccibruno wrote:
> shafik wrote:
> > Minor comment, does it make sense to covert this to a scoped enum since it 
> > looks like it is being strictly used as a set of values.
> Does it really add anything ?
> It means that instead of writing `CharacterLiteral::UTF32`
> you have to write `CharacterLiteral::CharacterKind::UTF32`
> 
> Seems a bit verbose. But I don't have any strong opinion on this.
It adds the protection against unintended conversions to and from the scoped 
enum, which in general is a good thing. The other benefits is not having the 
enumerators leak out into the surrounding scope but is limited in this case.

It does add verbosity though.


Repository:
  rC Clang

https://reviews.llvm.org/D54324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: include/clang/AST/Expr.h:1407
 public:
-  enum CharacterKind {
-Ascii,
-Wide,
-UTF8,
-UTF16,
-UTF32
-  };
+  enum CharacterKind { Ascii, Wide, UTF8, UTF16, UTF32 };
 

Minor comment, does it make sense to covert this to a scoped enum since it 
looks like it is being strictly used as a set of values.


Repository:
  rC Clang

https://reviews.llvm.org/D54324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added subscribers: rsmith, shafik.
shafik added a comment.
Herald added a reviewer: shafik.
Herald added a subscriber: gamesh411.

I think these changes make sense at a high level but I am not sure about the 
refactoring strategy. I am especially concerned we may end up in place where 
all the effected users of the API don't get updated and we are stuck with this 
parallel API.

Tagging in @rsmith since he has reviewed a lot of recent changes involving 
ASTImpoter that I have seen recently and he will have a better feeling for how 
these types of refactoring on handled on the clang side. I am mostly focused on 
the lldb side but care about the ASTImporter since we depend on it.


Repository:
  rC Clang

https://reviews.llvm.org/D53751



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56651: [ASTImporter] Fix importing OperatorDelete from CXXConstructorDecl

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Thank you, this looks good but perhaps some refactoring would help improve the 
change.




Comment at: lib/AST/ASTImporter.cpp:3243
+
+  if (R) {
+CXXDestructorDecl *ToDtor = cast(*R);

a_sidorin wrote:
> It's better to move this code to VisitFunctionDecl to keep all related stuff 
> together.
I am not sure I agree. Having everything in `VisitFunctionDecl` makes this code 
harder to maintain. 





Comment at: lib/AST/ASTImporter.cpp:3246
+
+auto Imp = importSeq(const_cast(D->getOperatorDelete()),
+ D->getOperatorDeleteThisArg());

a_sidorin wrote:
> Moving this code to VisitFunctionDecl will also allow us not to create a dtor 
> if this import fails.
At what point in `VisitFunctionDecl` would you expect this change to bail out 
of? A possible refactor would be to move this code to a separate function and 
then call that function at that point in `VisitFunctionDecl`. This would 
prevent `VisitFunctionDecl` from becoming even larger and I think address your 
concern.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56651



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: martong, teemperor, balazske, aaron.ballman.
Herald added a subscriber: rnkovacs.

When importing classes we may add a CXXMethodDecl more than once to a 
CXXRecordDecl. This patch will fix the cases we currently know about and handle 
both the case where we are only dealing with declarations and when we have to 
merge a definition into an existing declaration.


https://reviews.llvm.org/D56936

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2233,6 +2233,174 @@
 }).match(ToTU, functionDecl()));
 }
 
+TEST_P(ImportFunctions, ImportOverriddenMethodTwice) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto BP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *D = FirstDeclMatcher().match(FromTU0, DP);
+  Import(D, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *B = FirstDeclMatcher().match(FromTU1, BP);
+  Import(B, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DP), 1u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceDefinitionFirst) {
+  auto CodeWithOutDef =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeWithDef =
+  R"(
+struct B { virtual void f(){}; };
+struct D:B { void f(){}; };
+  )";
+  auto BP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+  auto BFDef = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFDef = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto FDefAll = cxxMethodDecl(hasName("f"), isDefinition());
+
+  Decl *ImportedD;
+  {
+Decl *FromTU = getTuDecl(CodeWithDef, Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(FromTU, DP);
+ImportedD = Import(FromD, Lang_CXX);
+  }
+  {
+Decl *FromTU = getTuDecl(CodeWithOutDef, Lang_CXX, "input1.cc");
+auto *FromB = FirstDeclMatcher().match(FromTU, BP);
+Import(FromB, Lang_CXX);
+  }
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFDef), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFDef), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, FDefAll), 2u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDef) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){};
+  )";
+
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto BFPIsDef = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *D = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(D, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *B = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(B, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFPIsDef), 0u);
+
+  auto *ToB = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("B")));
+  auto *ToBFInClass = FirstDeclMatcher().match(ToTU, BFP);
+  auto *ToBFOutOfClass = FirstDeclMatcher().match(
+  ToTU, cxxMethodDecl(hasName("f"), isDefinition()));
+
+  // The definition should be out-of-class.
+  EXPECT_NE(ToBFInClass, ToBFOutOfClass);
+  EXPECT_NE(ToBFInClass->getLexicalDeclContext(),
+ToBFOutOfClass->getLexicalDeclContext());
+  EXPECT_EQ(ToBFOutOfClass->getDeclContext(), ToB);
+  EXPECT_EQ(ToBFOutOfClass->getLexicalDeclContext(), ToTU);
+
+  // Check that the redecl chain is intact.
+  EXPECT_EQ(ToBFOutOfClass->getPreviousDecl(), ToBFInClass);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDefTwoTUs) {
+  auto CodeTU0 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeTU1 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){}
+  void D::f(){}
+  void foo(B ) { b.f(); }
+  )";
+
+  auto BFP =
+  

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

@martong the only issue is that I am seeing a regression on 
`Analysis/ctu-main.cpp` when I run `check-clang`. I am going to look into it 
but if you have any insights that would be helpful.


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

https://reviews.llvm.org/D56936



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: lib/AST/ASTStructuralEquivalence.cpp:302
+/// is inspired by ASTContext::mergeFunctionTypes(), we compare calling
+/// conventions bits but must not compare some other bits, e.g. the noreturn
+/// bit.

This comment is confusing b/c it looks like the noreturn bits are the only one 
you are not checking.



Comment at: unittests/AST/StructuralEquivalenceTest.cpp:373
 
+TEST_F(StructuralEquivalenceFunctionTest,
+FunctionsWithDifferentAttributesButSameTypesShouldBeEqual) {

Can we get some more tests to be a little more thorough and can we also get a 
test where it is expected to to be false as well?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53699



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Sounds good!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53755



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik requested changes to this revision.
shafik added a comment.
This revision now requires changes to proceed.

The rest of the changes look good.




Comment at: lib/AST/ExternalASTMerger.cpp:147
   ToTag->setHasExternalLexicalStorage();
-  ToTag->setMustBuildLookupTable();
+  ToTag->getPrimaryContext()->setMustBuildLookupTable();
   assert(Parent.CanComplete(ToTag));

This change looks unrelated to the refactoring of `GetAlreadyImportedOrNull()` 
so should probably be in another PR



Comment at: lib/AST/ExternalASTMerger.cpp:154
   ToContainer->setHasExternalLexicalStorage();
-  ToContainer->setMustBuildLookupTable();
+  ToContainer->getPrimaryContext()->setMustBuildLookupTable();
   assert(Parent.CanComplete(ToContainer));

martong wrote:
> a_sidorin wrote:
> > Do we have to do the same for NamespaceDecls?
> Yes, I think so.
> The primary context of a `NamespaceDecl` can be other than itself:
> ```
> DeclContext *DeclContext::getPrimaryContext() {
>   switch (DeclKind) {
>   case Decl::TranslationUnit:
>   case Decl::ExternCContext:
>   case Decl::LinkageSpec:
>   case Decl::Export:
>   case Decl::Block:
>   case Decl::Captured:
>   case Decl::OMPDeclareReduction:
> // There is only one DeclContext for these entities.
> return this;
> 
>   case Decl::Namespace:
> // The original namespace is our primary context.
> return static_cast(this)->getOriginalNamespace();
> ```
> Consequently, we should call `setMustBuildLookupTable` only on a 
> `NamespaceDecl` if we know for sure that is a primary context.
This change looks unrelated to the refactoring of `GetAlreadyImportedOrNull()` 
so should probably be in another PR


Repository:
  rC Clang

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

https://reviews.llvm.org/D53755



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54898: Set MustBuildLookupTable on PrimaryContext in ExternalASTMerger

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Apologies, I meant to make the comment in the child PR


Repository:
  rC Clang

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

https://reviews.llvm.org/D54898



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53693: [ASTImporter] Typedef import brings in the complete type

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Apologies for my late comments.




Comment at: cfe/trunk/unittests/AST/ASTImporterTest.cpp:3780
+  typedefNameDecl(hasName("U")));
+  ASSERT_TRUE(ToD->getUnderlyingType()->isIncompleteType());
+

As far as I can tell `S` is complete in `Code`, am I missing something? 



Comment at: cfe/trunk/unittests/AST/ASTImporterTest.cpp:3792
+  typedefNameDecl(hasName("U")));
+  ASSERT_FALSE(FromD->getUnderlyingType()->isIncompleteType());
+

Given my previous comment, I don't see why the addition code would change where 
`S` was complete or not.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53693



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D53751#1311037 , @martong wrote:

> > I didn't actually see this comment get addressed other than to say it won't 
> > be a problem in practice, which I'm not certain I agree with. Was there a 
> > reason why this got commit before finding out if the reviewer with the 
> > concern agrees with your rationale? FWIW, I share the concern that having 
> > parallel APIs for any length of time is a dangerous thing. Given that 
> > "almost ready to go" is not "ready to go" but there's not an imminent 
> > release, I don't understand the rush to commit this.
>
> @aaron.ballman Thanks for your time and review on this.
>
> I completely understand you concern and agree that having such parallel API 
> even for a short time is not good. Please let me explain why we chose to do 
> this still:
>  `ASTImporter::Import` functions are used externally by LLDB and CTU as 
> clients. However, the functions are used internally too, inside `ASTImporter` 
> and `ASTNodeImporter`.  E.g. when we import an expression, then first we use 
> the `Import(QualType)` function to import its type.
>  Our goal is twofold: (1) enforce `ASTImporter` and `ASTNodeImporter` 
> implementation functions to always check the result of used import functions 
> and (2) make sure that clients can have detailed error information, so e.g. 
> in case of CTU we can handle unsupported constructs differently than ODR 
> errors.
>  As @balazske mentioned we could have changed the API in one lockstep but the 
> impact would have been too huge.


I believe an alternate approach would have been to change one function at a 
time and do this change across. These would have been smaller changes and 
easier to review. This would have prevented the need for an intermediate name 
which causes churn in the commit history.

> In the context of this patch, you can think of the newly introduced 
> `Import_New` functions as the internal only functions. I was thinking about 
> that we could make them private and `ASTNodeImporter` as a friend,  that way 
> we could hide them from clients and then the dual API would cease to exist.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53751



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54898: Set MustBuildLookupTable on PrimaryContext in ExternalASTMerger

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

As pointed out in this comment in another review

https://reviews.llvm.org/D44100#1311687

We need to make sure we are running the lldb test suite before committing and 
watching the bots to make sure the commit does not break them.

Thank you


Repository:
  rC Clang

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

https://reviews.llvm.org/D54898



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

As pointed out in this comment in another review

https://reviews.llvm.org/D44100#1311687

We need to make sure we are running the lldb test suite before committing and 
watching the bots to make sure the commit does not break them.

The change is not purely a refactor since previously the error was consumed.

Thank you


Repository:
  rC Clang

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

https://reviews.llvm.org/D53755



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56581: [ASTImporter] Set the described template if not set

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Can you add a test?


Repository:
  rC Clang

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

https://reviews.llvm.org/D56581



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

LGTM

Thank you for adding the additional test.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53699



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56581: [ASTImporter] Set the described template if not set

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

@martong Unfortunately this causes a regression on one of the lldb tests 
`TestDataFormatterLibcxxVector.py` e.g.

  lldb-dotest -p TestDataFormatterLibcxxVector.py -t -G gmodules 
--no-multiprocess 

So this is specific to modules. If you can't reproduce this I can provide a 
back-trace.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56581



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 183406.
shafik marked 3 inline comments as done.
shafik added a comment.

Changes based on comments

- Refactoring ImportFunctionDeclBody to return Error
- Refactoring test/Analysis/ctu-main.cpp based on Gabor's patch
- Removing merging of function body for later PR


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

https://reviews.llvm.org/D56936

Files:
  lib/AST/ASTImporter.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/ctu-main.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2233,6 +2233,191 @@
 }).match(ToTU, functionDecl()));
 }
 
+TEST_P(ImportFunctions, ImportOverriddenMethodTwice) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *DF = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(DF, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *BF = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(BF, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFP), 1u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceDefinitionFirst) {
+  auto CodeWithoutDef =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeWithDef =
+  R"(
+struct B { virtual void f(){}; };
+struct D:B { void f(){}; };
+  )";
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+  auto BFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("D"))), isDefinition());
+  auto FDefAllP = cxxMethodDecl(hasName("f"), isDefinition());
+
+  {
+Decl *FromTU = getTuDecl(CodeWithDef, Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(FromTU, DFP);
+Import(FromD, Lang_CXX);
+  }
+  {
+Decl *FromTU = getTuDecl(CodeWithoutDef, Lang_CXX, "input1.cc");
+auto *FromB = FirstDeclMatcher().match(FromTU, BFP);
+Import(FromB, Lang_CXX);
+  }
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFDefP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFDefP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, FDefAllP), 2u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDef) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){};
+  )";
+
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto BFPIsDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D"))),
+   unless(isDefinition()));
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *D = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(D, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *B = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(B, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFPIsDefP), 0u);
+
+  auto *ToB = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("B")));
+  auto *ToBFInClass = FirstDeclMatcher().match(ToTU, BFP);
+  auto *ToBFOutOfClass = FirstDeclMatcher().match(
+  ToTU, cxxMethodDecl(hasName("f"), isDefinition()));
+
+  // The definition should be out-of-class.
+  EXPECT_NE(ToBFInClass, ToBFOutOfClass);
+  EXPECT_NE(ToBFInClass->getLexicalDeclContext(),
+ToBFOutOfClass->getLexicalDeclContext());
+  EXPECT_EQ(ToBFOutOfClass->getDeclContext(), ToB);
+  EXPECT_EQ(ToBFOutOfClass->getLexicalDeclContext(), ToTU);
+
+  // Check that the redecl chain is intact.
+  EXPECT_EQ(ToBFOutOfClass->getPreviousDecl(), ToBFInClass);
+}
+
+TEST_P(ImportFunctions,
+   ImportOverriddenMethodTwiceOutOfClassDefInSeparateCode) {
+  auto CodeTU0 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeTU1 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void 

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

@martong so I just tested your `lit-tests.patch` and it looks good!




Comment at: lib/AST/ASTImporter.cpp:2959
+}
+
 ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {

balazske wrote:
> Code formatting is not OK in this function (and other places). (Indentation 
> and spaces around "(" ")".)
Apologies, I forgot to run clang-format


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

https://reviews.llvm.org/D56936



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik marked an inline comment as done.
shafik added a comment.

@martong your idea does not work b/c default construction `DeclarationName()` 
treats it the same as being empty. So `if (!Name)` is still `true`.




Comment at: lib/AST/ASTImporter.cpp:2463
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),

a_sidorin wrote:
> If I understand correctly, this will replace Name with SearchName causing an 
> anonymous enum to be imported as a named a few lines below. It doesn't look 
> like a correct behaviour to me.
This is correct because either `SearchName` is `Name` or it is the name of the 
typedef for the anonymous enum set via `ImportInto(SearchName, 
D->getTypedefNameForAnonDecl()->getDeclName())`


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

https://reviews.llvm.org/D59665



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 192466.
shafik added a comment.

Fixes based on comments.


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

https://reviews.llvm.org/D59845

Files:
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1946,6 +1946,12 @@
 }
 
 bool ASTNodeImporter::IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToEnum) {
+  // Eliminate a potential failure point where we attempt to re-import
+  // something we're trying to import while completin ToEnum
+  if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum))
+if (auto *ToOriginEnum = dyn_cast(ToOrigin))
+ToEnum = ToOriginEnum;
+
   StructuralEquivalenceContext Ctx(
   Importer.getFromContext(), Importer.getToContext(),
   Importer.getNonEquivalentDecls(), 
getStructuralEquivalenceKind(Importer));


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1946,6 +1946,12 @@
 }
 
 bool ASTNodeImporter::IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToEnum) {
+  // Eliminate a potential failure point where we attempt to re-import
+  // something we're trying to import while completin ToEnum
+  if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum))
+if (auto *ToOriginEnum = dyn_cast(ToOrigin))
+ToEnum = ToOriginEnum;
+
   StructuralEquivalenceContext Ctx(
   Importer.getFromContext(), Importer.getToContext(),
   Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik marked 2 inline comments as done.
shafik added inline comments.



Comment at: lib/AST/ASTImporter.cpp:1951
+  // something we're trying to import while completin ToEnum
+  Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum);
+

JDevlieghere wrote:
> ```
> if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum)) 
>   if (auto *ToOriginEnum = dyn_cast(ToOrigin))
> ToEnum = ToOriginEnum;
> ```
I normally just match the local style but this is indeed much better style.


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

https://reviews.llvm.org/D59845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
shafik marked an inline comment as done.
Closed by commit rL357100: [ASTImporter] Fix IsStructuralMatch specialization 
for EnumDecl to prevent re… (authored by shafik, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59845?vs=192466=192475#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59845

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -1946,6 +1946,12 @@
 }
 
 bool ASTNodeImporter::IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToEnum) {
+  // Eliminate a potential failure point where we attempt to re-import
+  // something we're trying to import while completin ToEnum
+  if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum))
+if (auto *ToOriginEnum = dyn_cast(ToOrigin))
+ToEnum = ToOriginEnum;
+
   StructuralEquivalenceContext Ctx(
   Importer.getFromContext(), Importer.getToContext(),
   Importer.getNonEquivalentDecls(), 
getStructuralEquivalenceKind(Importer));


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -1946,6 +1946,12 @@
 }
 
 bool ASTNodeImporter::IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToEnum) {
+  // Eliminate a potential failure point where we attempt to re-import
+  // something we're trying to import while completin ToEnum
+  if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum))
+if (auto *ToOriginEnum = dyn_cast(ToOrigin))
+ToEnum = ToOriginEnum;
+
   StructuralEquivalenceContext Ctx(
   Importer.getFromContext(), Importer.getToContext(),
   Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik marked an inline comment as done.
shafik added a comment.

In D59665#1442826 , @martong wrote:

> In D59665#1442328 , @shafik wrote:
>
> > @martong your idea does not work b/c default construction 
> > `DeclarationName()` treats it the same as being empty. So `if (!Name)` is 
> > still `true`.
>
>
> And did you try moving the `push_back` to the other scope? I'd expect the the 
> ConflictingDecls to be empty then.


Yes, I did and I think it looks good but I have to run a regression.




Comment at: lib/AST/ASTImporter.cpp:2463
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),

martong wrote:
> shafik wrote:
> > a_sidorin wrote:
> > > If I understand correctly, this will replace Name with SearchName causing 
> > > an anonymous enum to be imported as a named a few lines below. It doesn't 
> > > look like a correct behaviour to me.
> > This is correct because either `SearchName` is `Name` or it is the name of 
> > the typedef for the anonymous enum set via `ImportInto(SearchName, 
> > D->getTypedefNameForAnonDecl()->getDeclName())`
> Okay, this is indeed correct. But then we should make a similar change in 
> VisitRecordDecl too (there we do exactly the same with typedefs).
This is fixing a specific bug, so I would want to keep this change specifically 
related to that and I can add a second review for `VisitRecordDecl`


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

https://reviews.llvm.org/D59665



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D59665#1439070 , @martong wrote:

> Hi Shafik,
>
> Thank you for looking into this. This is indeed a bug, because whenever a we 
> encounter an unnamed EnumDecl in the "from" context then we return with a 
> nameconflict error.
>  However, I think we should fix this differently.
>  First of all, currently HandleNameConflict just returns the parameter `Name` 
> it received. This may be unnamed in some cases and thus converts to true. So 
> I think HandleNameConflict should be changed that it would return a default 
> initialized DeclarationName; so once we have anything in the ConflictingDecls 
> then we return with the NameConflict error:
>
>   DeclarationName ASTImporter::HandleNameConflict(DeclarationName Name,
>   DeclContext *DC,
>   unsigned IDNS,
>   NamedDecl **Decls,
>   unsigned NumDecls) {
>   -  return Name;
>   +  return DeclarationName();
>   }
>
>
> And most importantly, I think we should push into the ConflictingDecls only 
> if the structural match indicates a non-match:
>
> if (auto *FoundEnum = dyn_cast(FoundDecl)) {
>   if (isStructuralMatch(D, FoundEnum, true))
> return Importer.MapImported(D, FoundEnum);
>   +   ConflictingDecls.push_back(FoundDecl);
> }
>   -
>   - ConflictingDecls.push_back(FoundDecl);
>
>
> I am aware of similar errors like this with other AST nodes. We had a patch 
> in our fork to fix that in January 
> (https://github.com/Ericsson/clang/pull/569/files) I am going to prepare a 
> patch from that cause I see now this affects you guys in LLDB too.


This sounds reasonable, let me test it out and make sure it looks good.


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

https://reviews.llvm.org/D59665



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59761: [ASTImporter] Convert ODR diagnostics inside ASTImporter implementation

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D59761



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

LLDB regression test that goes with this fix: https://reviews.llvm.org/D59847


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

https://reviews.llvm.org/D59845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: martong, teemperor, friss, a_sidorin.
Herald added subscribers: jdoerfert, rnkovacs.

We may try and re-import an EnumDecl while trying to complete it in 
`IsStructuralMatch(...)` specialization for `EnumDecl`. This change mirrors a 
similar fix for the specialization for `RecordDecl`.


https://reviews.llvm.org/D59845

Files:
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1946,6 +1946,17 @@
 }
 
 bool ASTNodeImporter::IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToEnum) {
+  // Eliminate a potential failure point where we attempt to re-import
+  // something we're trying to import while completin ToEnum
+  Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum);
+
+  if (ToOrigin) {
+auto *ToOriginEnum = dyn_cast(ToOrigin);
+
+if (ToOriginEnum)
+ToEnum = ToOriginEnum;
+  }
+
   StructuralEquivalenceContext Ctx(
   Importer.getFromContext(), Importer.getToContext(),
   Importer.getNonEquivalentDecls(), 
getStructuralEquivalenceKind(Importer));


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1946,6 +1946,17 @@
 }
 
 bool ASTNodeImporter::IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToEnum) {
+  // Eliminate a potential failure point where we attempt to re-import
+  // something we're trying to import while completin ToEnum
+  Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum);
+
+  if (ToOrigin) {
+auto *ToOriginEnum = dyn_cast(ToOrigin);
+
+if (ToOriginEnum)
+ToEnum = ToOriginEnum;
+  }
+
   StructuralEquivalenceContext Ctx(
   Importer.getFromContext(), Importer.getToContext(),
   Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D58743#1413249 , @lebedev.ri wrote:

> Is there a test missing here?


This origin of this fix is the LLDB expression parsing issue. We were not able 
to reduce to a test we could put in `ASTImpoterTest.cpp` but we have a LLDB 
test that exercises this issue and will pass once this fix is in place.

See this differential https://reviews.llvm.org/D58790




Comment at: lib/AST/ASTImporter.cpp:8284
+
+if (!isBuiltin) {
+   // Include location of this file.

balazske wrote:
> The `Cache` can be moved into this block and the block to `else if`.
I am not sure I understand what you are asking. Do you mean just duplicate the 

const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();

in both blocks?



Comment at: lib/AST/ASTImporter.cpp:8301
+ToID = ToSM.createFileID(Entry, ToIncludeLoc,
+ 
FromSLoc.getFile().getFileCharacteristic());
+}

balazske wrote:
> Is it possible that in the `isBuiltin` true case this part of the code does 
> run (always) without assertion or other error and the result is always an 
> invalid `ToID`? (If yes the whole change is not needed, so I want to know 
> what was the reason for this change, was there any crash or bug.)
This change was the result of a assert where evaluating an expression resulted 
in the built-in being imported and it would assert in `SourceManager` when when 
`Import(SourceLocation)`   attempted `getDecomposedLoc`  at the next step.

AFAICT the The Preprocessor sets it up the buffer 
[here](https://github.com/llvm-mirror/clang/blob/master/lib/Lex/Preprocessor.cpp#L552)
 and then creates the `FileID` for in the `SourceManager` and copying over the 
buffer should be the correct thing to do since that seems to be sufficient. 

I also while testing verified that the `BufferIndetifer()` did indeed equal 
`""` similar to [this 
check](https://github.com/llvm-mirror/clang/blob/master/lib/Basic/SourceManager.cpp#L2009)
 in `SourceManager`.



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

https://reviews.llvm.org/D58743



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 188753.
shafik marked 7 inline comments as done.
shafik added a comment.

Addressed comments on formatting and missing changes to the `Import_New` 
version of the code.


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

https://reviews.llvm.org/D58743

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -8229,9 +8229,10 @@
 return {};
 
   SourceManager  = FromContext.getSourceManager();
+  bool IsBuiltin = FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::pair Decomposed = FromSM.getDecomposedLoc(FromLoc);
-  FileID ToFileID = Import(Decomposed.first);
+  FileID ToFileID = Import(Decomposed.first, IsBuiltin);
   if (ToFileID.isInvalid())
 return {};
   SourceManager  = ToContext.getSourceManager();
@@ -8246,13 +8247,13 @@
   return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
 }
 
-Expected ASTImporter::Import_New(FileID FromID) {
-  FileID ToID = Import(FromID);
+Expected ASTImporter::Import_New(FileID FromID, bool IsBuiltin) {
+  FileID ToID = Import(FromID, IsBuiltin);
   if (ToID.isInvalid() && FromID.isValid())
 return llvm::make_error();
   return ToID;
 }
-FileID ASTImporter::Import(FileID FromID) {
+FileID ASTImporter::Import(FileID FromID, bool IsBuiltin) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
 return Pos->second;
@@ -8278,25 +8279,29 @@
 }
 ToID = ToSM.getFileID(MLoc);
   } else {
-// Include location of this file.
-SourceLocation ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
-
 const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();
-if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
-  // FIXME: We probably want to use getVirtualFile(), so we don't hit the
-  // disk again
-  // FIXME: We definitely want to re-use the existing MemoryBuffer, rather
-  // than mmap the files several times.
-  const FileEntry *Entry =
-  ToFileManager.getFile(Cache->OrigEntry->getName());
-  // FIXME: The filename may be a virtual name that does probably not
-  // point to a valid file and we get no Entry here. In this case try with
-  // the memory buffer below.
-  if (Entry)
-ToID = ToSM.createFileID(Entry, ToIncludeLoc,
- FromSLoc.getFile().getFileCharacteristic());
+
+if (!IsBuiltin) {
+  // Include location of this file.
+  SourceLocation ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
+
+  if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
+// FIXME: We probably want to use getVirtualFile(), so we don't hit the
+// disk again
+// FIXME: We definitely want to re-use the existing MemoryBuffer, 
rather
+// than mmap the files several times.
+const FileEntry *Entry =
+ToFileManager.getFile(Cache->OrigEntry->getName());
+// FIXME: The filename may be a virtual name that does probably not
+// point to a valid file and we get no Entry here. In this case try 
with
+// the memory buffer below.
+if (Entry)
+  ToID = ToSM.createFileID(Entry, ToIncludeLoc,
+   FromSLoc.getFile().getFileCharacteristic());
+  }
 }
-if (ToID.isInvalid()) {
+
+if (ToID.isInvalid() || IsBuiltin) {
   // FIXME: We want to re-use the existing MemoryBuffer!
   bool Invalid = true;
   const llvm::MemoryBuffer *FromBuf = Cache->getBuffer(
Index: include/clang/AST/ASTImporter.h
===
--- include/clang/AST/ASTImporter.h
+++ include/clang/AST/ASTImporter.h
@@ -337,9 +337,9 @@
 ///
 /// \returns The equivalent file ID in the source manager of the "to"
 /// context, or the import error.
-llvm::Expected Import_New(FileID);
+llvm::Expected Import_New(FileID, bool IsBuiltin = false);
 // FIXME: Remove this version.
-FileID Import(FileID);
+FileID Import(FileID, bool IsBuiltin = false);
 
 /// Import the given C++ constructor initializer from the "from"
 /// context into the "to" context.


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -8229,9 +8229,10 @@
 return {};
 
   SourceManager  = FromContext.getSourceManager();
+  bool IsBuiltin = FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::pair Decomposed = FromSM.getDecomposedLoc(FromLoc);
-  FileID ToFileID = Import(Decomposed.first);
+  FileID ToFileID = Import(Decomposed.first, IsBuiltin);
   if (ToFileID.isInvalid())
 return {};
   SourceManager  = ToContext.getSourceManager();
@@ -8246,13 +8247,13 @@
   return 

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

@teemperor ping


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

https://reviews.llvm.org/D58743



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58830: [ASTImporter] Import member expr with explicit template args

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM and I don't see any regressions.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58830



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM outside of the question I had.




Comment at: lib/AST/ASTImporter.cpp:4967
+template  static auto getTemplateDefinition(T *D) -> T * {
+  auto *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
   if (!ToTemplatedDef)

Can we guarantee that `D->getTemplatedDecl()` will always return a valid 
pointer? 



Comment at: lib/AST/ASTImporter.cpp:4967
+template  static auto getTemplateDefinition(T *D) -> T * {
+  auto *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
   if (!ToTemplatedDef)

shafik wrote:
> Can we guarantee that `D->getTemplatedDecl()` will always return a valid 
> pointer? 
What other types besides `CXXRecordDecl` do we expect here? 



Comment at: lib/AST/ASTImporter.cpp:5544
   // type, and in the same context as the function we're importing.
+  // FIXME Split this into a separate function.
   if (!LexicalDC->isFunctionOrMethod()) {

Would it make sense to do the split into a separate function in the PR?



Comment at: lib/AST/ASTImporter.cpp:5595
+  auto *PrevTemplated =
+  FoundByLookup->getTemplatedDecl()->getMostRecentDecl();
+  if (TemplatedFD != PrevTemplated)

Can we guarantee that `FoundByLookup->getTemplatedDecl()` will always return a 
valid pointer? 


Repository:
  rC Clang

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

https://reviews.llvm.org/D58494



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: lib/AST/ASTImporter.cpp:5147
   }
+} else { // ODR violation.
+  // FIXME HandleNameConflict

ODR violations are ill-formed no diagnostic required. So currently will this 
fail for cases that clang proper would not?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58502: [ASTImporter] Fix redecl failures of Class and ClassTemplate

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM, thank you !


Repository:
  rC Clang

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

https://reviews.llvm.org/D58502



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL355332: [ASTImporter] Handle built-in when importing 
SourceLocation and FileID (authored by shafik, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58743?vs=188753=189185#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58743

Files:
  cfe/trunk/include/clang/AST/ASTImporter.h
  cfe/trunk/lib/AST/ASTImporter.cpp


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -8229,9 +8229,10 @@
 return {};
 
   SourceManager  = FromContext.getSourceManager();
+  bool IsBuiltin = FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::pair Decomposed = FromSM.getDecomposedLoc(FromLoc);
-  FileID ToFileID = Import(Decomposed.first);
+  FileID ToFileID = Import(Decomposed.first, IsBuiltin);
   if (ToFileID.isInvalid())
 return {};
   SourceManager  = ToContext.getSourceManager();
@@ -8246,13 +8247,13 @@
   return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
 }
 
-Expected ASTImporter::Import_New(FileID FromID) {
-  FileID ToID = Import(FromID);
+Expected ASTImporter::Import_New(FileID FromID, bool IsBuiltin) {
+  FileID ToID = Import(FromID, IsBuiltin);
   if (ToID.isInvalid() && FromID.isValid())
 return llvm::make_error();
   return ToID;
 }
-FileID ASTImporter::Import(FileID FromID) {
+FileID ASTImporter::Import(FileID FromID, bool IsBuiltin) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
 return Pos->second;
@@ -8278,25 +8279,29 @@
 }
 ToID = ToSM.getFileID(MLoc);
   } else {
-// Include location of this file.
-SourceLocation ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
-
 const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();
-if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
-  // FIXME: We probably want to use getVirtualFile(), so we don't hit the
-  // disk again
-  // FIXME: We definitely want to re-use the existing MemoryBuffer, rather
-  // than mmap the files several times.
-  const FileEntry *Entry =
-  ToFileManager.getFile(Cache->OrigEntry->getName());
-  // FIXME: The filename may be a virtual name that does probably not
-  // point to a valid file and we get no Entry here. In this case try with
-  // the memory buffer below.
-  if (Entry)
-ToID = ToSM.createFileID(Entry, ToIncludeLoc,
- FromSLoc.getFile().getFileCharacteristic());
+
+if (!IsBuiltin) {
+  // Include location of this file.
+  SourceLocation ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
+
+  if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
+// FIXME: We probably want to use getVirtualFile(), so we don't hit the
+// disk again
+// FIXME: We definitely want to re-use the existing MemoryBuffer, 
rather
+// than mmap the files several times.
+const FileEntry *Entry =
+ToFileManager.getFile(Cache->OrigEntry->getName());
+// FIXME: The filename may be a virtual name that does probably not
+// point to a valid file and we get no Entry here. In this case try 
with
+// the memory buffer below.
+if (Entry)
+  ToID = ToSM.createFileID(Entry, ToIncludeLoc,
+   FromSLoc.getFile().getFileCharacteristic());
+  }
 }
-if (ToID.isInvalid()) {
+
+if (ToID.isInvalid() || IsBuiltin) {
   // FIXME: We want to re-use the existing MemoryBuffer!
   bool Invalid = true;
   const llvm::MemoryBuffer *FromBuf = Cache->getBuffer(
Index: cfe/trunk/include/clang/AST/ASTImporter.h
===
--- cfe/trunk/include/clang/AST/ASTImporter.h
+++ cfe/trunk/include/clang/AST/ASTImporter.h
@@ -337,9 +337,9 @@
 ///
 /// \returns The equivalent file ID in the source manager of the "to"
 /// context, or the import error.
-llvm::Expected Import_New(FileID);
+llvm::Expected Import_New(FileID, bool IsBuiltin = false);
 // FIXME: Remove this version.
-FileID Import(FileID);
+FileID Import(FileID, bool IsBuiltin = false);
 
 /// Import the given C++ constructor initializer from the "from"
 /// context into the "to" context.


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -8229,9 +8229,10 @@
 return {};
 
   SourceManager  = FromContext.getSourceManager();
+  bool IsBuiltin = FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::pair Decomposed = 

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: martong, a.sidorin, teemperor, aaron.ballman.
Herald added subscribers: jdoerfert, rnkovacs.

Currently when we see a built-in we try and import the include location. 
Instead what we do now is find the buffer like we do for the invalid case and 
copy that over to the to context.


https://reviews.llvm.org/D58743

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -8229,9 +8229,10 @@
 return {};
 
   SourceManager  = FromContext.getSourceManager();
+  bool IsBuiltin =  FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::pair Decomposed = FromSM.getDecomposedLoc(FromLoc);
-  FileID ToFileID = Import(Decomposed.first);
+  FileID ToFileID = Import(Decomposed.first, IsBuiltin);
   if (ToFileID.isInvalid())
 return {};
   SourceManager  = ToContext.getSourceManager();
@@ -8252,7 +8253,7 @@
 return llvm::make_error();
   return ToID;
 }
-FileID ASTImporter::Import(FileID FromID) {
+FileID ASTImporter::Import(FileID FromID, bool isBuiltin) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
 return Pos->second;
@@ -8278,25 +8279,30 @@
 }
 ToID = ToSM.getFileID(MLoc);
   } else {
-// Include location of this file.
-SourceLocation ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
-
 const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();
-if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
-  // FIXME: We probably want to use getVirtualFile(), so we don't hit the
-  // disk again
-  // FIXME: We definitely want to re-use the existing MemoryBuffer, rather
-  // than mmap the files several times.
-  const FileEntry *Entry =
-  ToFileManager.getFile(Cache->OrigEntry->getName());
-  // FIXME: The filename may be a virtual name that does probably not
-  // point to a valid file and we get no Entry here. In this case try with
-  // the memory buffer below.
-  if (Entry)
-ToID = ToSM.createFileID(Entry, ToIncludeLoc,
- FromSLoc.getFile().getFileCharacteristic());
+
+if (!isBuiltin) {
+   // Include location of this file.
+   SourceLocation ToIncludeLoc = 
Import(FromSLoc.getFile().getIncludeLoc());
+
+
+if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
+  // FIXME: We probably want to use getVirtualFile(), so we don't hit 
the
+  // disk again
+  // FIXME: We definitely want to re-use the existing MemoryBuffer, 
rather
+  // than mmap the files several times.
+  const FileEntry *Entry =
+  ToFileManager.getFile(Cache->OrigEntry->getName());
+  // FIXME: The filename may be a virtual name that does probably not
+  // point to a valid file and we get no Entry here. In this case try 
with
+  // the memory buffer below.
+  if (Entry)
+ToID = ToSM.createFileID(Entry, ToIncludeLoc,
+ 
FromSLoc.getFile().getFileCharacteristic());
+}
 }
-if (ToID.isInvalid()) {
+
+if (ToID.isInvalid() || isBuiltin) {
   // FIXME: We want to re-use the existing MemoryBuffer!
   bool Invalid = true;
   const llvm::MemoryBuffer *FromBuf = Cache->getBuffer(
Index: include/clang/AST/ASTImporter.h
===
--- include/clang/AST/ASTImporter.h
+++ include/clang/AST/ASTImporter.h
@@ -339,7 +339,7 @@
 /// context, or the import error.
 llvm::Expected Import_New(FileID);
 // FIXME: Remove this version.
-FileID Import(FileID);
+FileID Import(FileID, bool isBuiltin=false);
 
 /// Import the given C++ constructor initializer from the "from"
 /// context into the "to" context.


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -8229,9 +8229,10 @@
 return {};
 
   SourceManager  = FromContext.getSourceManager();
+  bool IsBuiltin =  FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::pair Decomposed = FromSM.getDecomposedLoc(FromLoc);
-  FileID ToFileID = Import(Decomposed.first);
+  FileID ToFileID = Import(Decomposed.first, IsBuiltin);
   if (ToFileID.isInvalid())
 return {};
   SourceManager  = ToContext.getSourceManager();
@@ -8252,7 +8253,7 @@
 return llvm::make_error();
   return ToID;
 }
-FileID ASTImporter::Import(FileID FromID) {
+FileID ASTImporter::Import(FileID FromID, bool isBuiltin) {
   llvm::DenseMap::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
 return Pos->second;
@@ -8278,25 +8279,30 @@
 }
 ToID = ToSM.getFileID(MLoc);
   } else {
-// Include location of this file.
-

[PATCH] D57590: [ASTImporter] Improve import of FileID.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

LGTM I don't see any LLDB test regressions but I would prefer another reviewer 
as well.

I am going to be modifying this soon to fix some issues w/ handling built-ins.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57590



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D58668



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: teemperor, martong, a_sidorin.
Herald added a subscriber: rnkovacs.

https://reviews.llvm.org/D51633 added error handling to the 
`ASTNodeImporter::VisitEnumDecl(...)` for the conflicting names case. This 
could lead to erroneous return of an error in that case since we should have 
been using `SearchName`. `Name` may be empty in the case where we find the name 
via `getTypedefNameForAnonDecl(...)`.


https://reviews.llvm.org/D59665

Files:
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2460,7 +2460,7 @@
 }
 
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),
  ConflictingDecls.size());
   if (!Name)


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2460,7 +2460,7 @@
 }
 
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),
  ConflictingDecls.size());
   if (!Name)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I was not able to come up with a test that would detect this issue using either 
`clang-import-test` nor via any of the methods used in `ASTImpoterTest.cpp`. I 
created a regression test on the lldb side, which should pass once this is 
committed:

https://reviews.llvm.org/D59667


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

https://reviews.llvm.org/D59665



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53757: [ASTImporter] Changed use of Import to Import_New in ASTNodeImporter.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: lib/AST/ASTImporter.cpp:3418
 
-  for (const auto *Attr : D->attrs())
-ToIndirectField->addAttr(Importer.Import(Attr));

Why is this section of code removed?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53757



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57902: [AST] Fix structural inequivalence of operators

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: unittests/AST/StructuralEquivalenceTest.cpp:251
+
+TEST_F(StructuralEquivalenceFunctionTest, CtorVsDtor) {
+  auto t = makeDecls(

Curious, is this test just for completeness or is this somehow related to the 
overloaded operator check?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57902



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57740: [ASTImporter] Import every Decl in lambda record

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

LGTM although I wish we had more lambda tests.

Please remember to check the LLDB build bots when landing the patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57740



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: jdoerfert.

I ran `check-lldb` locally and it looks good.


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

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

This looks reasonable, I will wait for @martong and/or @a_sidorin to review.

FYI LLDB is the other big user of ASTImpoter so it is helpful if you can run 
`check-lldb` especially on MacOS so you can to catch regressions before 
committing. After committing please make sure to monitor the GreenDragon build 
bots:

  http://lab.llvm.org:8080/green/view/LLDB/

Thank you!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58292



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57910: [ASTImporter] Find previous friend function template

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

LGTM, I just ran `check-lldb` and I don't see any regressions.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57910



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57322: [ASTImporter] Refactor unittests to be able to parameterize them in a more flexible way

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

This looks reasonable to me but I don't have strong opinions on refactoring 
gtest tests.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57322



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57322: [ASTImporter] Refactor unittests to be able to parameterize them in a more flexible way

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: unittests/AST/ASTImporterTest.cpp:468
 
+  template  DeclT *Import(DeclT *From, Language Lang) {
+return cast_or_null(Import(cast(From), Lang));

Is this being used in this PR?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57322



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352436: [ASTImporter] Fix handling of overriden methods 
during ASTImport (authored by shafik, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D56936

Files:
  lib/AST/ASTImporter.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/ctu-main.cpp
  unittests/AST/ASTImporterTest.cpp

Index: test/Analysis/Inputs/ctu-other.cpp
===
--- test/Analysis/Inputs/ctu-other.cpp
+++ test/Analysis/Inputs/ctu-other.cpp
@@ -24,33 +24,38 @@
 int fens(int x) {
   return x - 3;
 }
-}
+} // namespace embed_ns
 
 class embed_cls {
 public:
-  int fecl(int x) {
-return x - 7;
-  }
+  int fecl(int x);
 };
+int embed_cls::fecl(int x) {
+  return x - 7;
 }
+} // namespace myns
 
 class mycls {
 public:
-  int fcl(int x) {
-return x + 5;
-  }
-  static int fscl(int x) {
-return x + 6;
-  }
+  int fcl(int x);
+  static int fscl(int x);
 
   class embed_cls2 {
   public:
-int fecl2(int x) {
-  return x - 11;
-}
+int fecl2(int x);
   };
 };
 
+int mycls::fcl(int x) {
+  return x + 5;
+}
+int mycls::fscl(int x) {
+  return x + 6;
+}
+int mycls::embed_cls2::fecl2(int x) {
+  return x - 11;
+}
+
 namespace chns {
 int chf2(int x);
 
Index: test/Analysis/ctu-main.cpp
===
--- test/Analysis/ctu-main.cpp
+++ test/Analysis/ctu-main.cpp
@@ -78,6 +78,6 @@
   clang_analyzer_eval(fun_using_anon_struct(8) == 8); // expected-warning{{TRUE}}
 
   clang_analyzer_eval(other_macro_diag(1) == 1); // expected-warning{{TRUE}}
-  // expected-warning@Inputs/ctu-other.cpp:75{{REACHABLE}}
+  // expected-warning@Inputs/ctu-other.cpp:80{{REACHABLE}}
   MACRODIAG(); // expected-warning{{REACHABLE}}
 }
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -437,6 +437,8 @@
 
 Error ImportTemplateInformation(FunctionDecl *FromFD, FunctionDecl *ToFD);
 
+Error ImportFunctionDeclBody(FunctionDecl *FromFD, FunctionDecl *ToFD);
+
 bool IsStructuralMatch(Decl *From, Decl *To, bool Complain);
 bool IsStructuralMatch(RecordDecl *FromRecord, RecordDecl *ToRecord,
bool Complain = true);
@@ -2944,6 +2946,17 @@
   return FoundSpec;
 }
 
+Error ASTNodeImporter::ImportFunctionDeclBody(FunctionDecl *FromFD,
+  FunctionDecl *ToFD) {
+  if (Stmt *FromBody = FromFD->getBody()) {
+if (ExpectedStmt ToBodyOrErr = import(FromBody))
+  ToFD->setBody(*ToBodyOrErr);
+else
+  return ToBodyOrErr.takeError();
+  }
+  return Error::success();
+}
+
 ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
 
   SmallVector Redecls = getCanonicalForwardRedeclChain(D);
@@ -2967,7 +2980,7 @@
   if (ToD)
 return ToD;
 
-  const FunctionDecl *FoundByLookup = nullptr;
+  FunctionDecl *FoundByLookup = nullptr;
   FunctionTemplateDecl *FromFT = D->getDescribedFunctionTemplate();
 
   // If this is a function template specialization, then try to find the same
@@ -3038,6 +3051,25 @@
 }
   }
 
+  // We do not allow more than one in-class declaration of a function. This is
+  // because AST clients like VTableBuilder asserts on this. VTableBuilder
+  // assumes there is only one in-class declaration. Building a redecl
+  // chain would result in more than one in-class declaration for
+  // overrides (even if they are part of the same redecl chain inside the
+  // derived class.)
+  if (FoundByLookup) {
+if (auto *MD = dyn_cast(FoundByLookup)) {
+  if (D->getLexicalDeclContext() == D->getDeclContext()) {
+if (!D->doesThisDeclarationHaveABody())
+  return Importer.MapImported(D, FoundByLookup);
+else {
+  // Let's continue and build up the redecl chain in this case.
+  // FIXME Merge the functions into one decl.
+}
+  }
+}
+  }
+
   DeclarationNameInfo NameInfo(Name, Loc);
   // Import additional name location/type info.
   if (Error Err = ImportDeclarationNameLoc(D->getNameInfo(), NameInfo))
@@ -3199,12 +3231,10 @@
   }
 
   if (D->doesThisDeclarationHaveABody()) {
-if (Stmt *FromBody = D->getBody()) {
-  if (ExpectedStmt ToBodyOrErr = import(FromBody))
-ToFunction->setBody(*ToBodyOrErr);
-  else
-return ToBodyOrErr.takeError();
-}
+Error Err = ImportFunctionDeclBody(D, ToFunction);
+
+if (Err)
+  return std::move(Err);
   }
 
   // FIXME: Other bits to merge?
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2232,6 +2232,191 @@
 }).match(ToTU, functionDecl()));
 }
 
+TEST_P(ImportFunctions, 

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 183923.
shafik marked 2 inline comments as done.
shafik added a comment.

Addressing comments on commenting and naming.


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

https://reviews.llvm.org/D56936

Files:
  lib/AST/ASTImporter.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/ctu-main.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2233,6 +2233,191 @@
 }).match(ToTU, functionDecl()));
 }
 
+TEST_P(ImportFunctions, ImportOverriddenMethodTwice) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *DF = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(DF, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *BF = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(BF, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFP), 1u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceDefinitionFirst) {
+  auto CodeWithoutDef =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeWithDef =
+  R"(
+struct B { virtual void f(){}; };
+struct D:B { void f(){}; };
+  )";
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+  auto BFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("D"))), isDefinition());
+  auto FDefAllP = cxxMethodDecl(hasName("f"), isDefinition());
+
+  {
+Decl *FromTU = getTuDecl(CodeWithDef, Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(FromTU, DFP);
+Import(FromD, Lang_CXX);
+  }
+  {
+Decl *FromTU = getTuDecl(CodeWithoutDef, Lang_CXX, "input1.cc");
+auto *FromB = FirstDeclMatcher().match(FromTU, BFP);
+Import(FromB, Lang_CXX);
+  }
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFDefP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFDefP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, FDefAllP), 2u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDef) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){};
+  )";
+
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto BFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D"))),
+   unless(isDefinition()));
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *D = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(D, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *B = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(B, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFDefP), 0u);
+
+  auto *ToB = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("B")));
+  auto *ToBFInClass = FirstDeclMatcher().match(ToTU, BFP);
+  auto *ToBFOutOfClass = FirstDeclMatcher().match(
+  ToTU, cxxMethodDecl(hasName("f"), isDefinition()));
+
+  // The definition should be out-of-class.
+  EXPECT_NE(ToBFInClass, ToBFOutOfClass);
+  EXPECT_NE(ToBFInClass->getLexicalDeclContext(),
+ToBFOutOfClass->getLexicalDeclContext());
+  EXPECT_EQ(ToBFOutOfClass->getDeclContext(), ToB);
+  EXPECT_EQ(ToBFOutOfClass->getLexicalDeclContext(), ToTU);
+
+  // Check that the redecl chain is intact.
+  EXPECT_EQ(ToBFOutOfClass->getPreviousDecl(), ToBFInClass);
+}
+
+TEST_P(ImportFunctions,
+   ImportOverriddenMethodTwiceOutOfClassDefInSeparateCode) {
+  auto CodeTU0 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeTU1 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){}
+  void D::f(){}
+  void foo(B , D ) { b.f(); d.f(); }
+  )";
+
+  auto BFP =
+  cxxMethodDecl(hasName("f"), 

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 183251.
shafik added a comment.

Addressing comments on naming in the unit test and refactoring of duplicated 
code.


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

https://reviews.llvm.org/D56936

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2233,6 +2233,187 @@
 }).match(ToTU, functionDecl()));
 }
 
+TEST_P(ImportFunctions, ImportOverriddenMethodTwice) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto BFP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DFP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *DF = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(DF, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *BF = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(BF, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFP), 1u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceDefinitionFirst) {
+  auto CodeWithoutDef =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeWithDef =
+  R"(
+struct B { virtual void f(){}; };
+struct D:B { void f(){}; };
+  )";
+  auto BFP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto DFP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D";
+  auto BFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("D"))), isDefinition());
+  auto FDefAllP = cxxMethodDecl(hasName("f"), isDefinition());
+
+  {
+Decl *FromTU = getTuDecl(CodeWithDef, Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(FromTU, DFP);
+Import(FromD, Lang_CXX);
+  }
+  {
+Decl *FromTU = getTuDecl(CodeWithoutDef, Lang_CXX, "input1.cc");
+auto *FromB = FirstDeclMatcher().match(FromTU, BFP);
+Import(FromB, Lang_CXX);
+  }
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFDefP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, DFDefP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, FDefAllP), 2u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDef) {
+  auto Code =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){};
+  )";
+
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto BFPIsDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D"))), unless(isDefinition()) );
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *D = FirstDeclMatcher().match(FromTU0, DFP);
+  Import(D, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *B = FirstDeclMatcher().match(FromTU1, BFP);
+  Import(B, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter().match(ToTU, BFPIsDefP), 0u);
+
+  auto *ToB = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("B")));
+  auto *ToBFInClass = FirstDeclMatcher().match(ToTU, BFP);
+  auto *ToBFOutOfClass = FirstDeclMatcher().match(
+  ToTU, cxxMethodDecl(hasName("f"), isDefinition()));
+
+  // The definition should be out-of-class.
+  EXPECT_NE(ToBFInClass, ToBFOutOfClass);
+  EXPECT_NE(ToBFInClass->getLexicalDeclContext(),
+ToBFOutOfClass->getLexicalDeclContext());
+  EXPECT_EQ(ToBFOutOfClass->getDeclContext(), ToB);
+  EXPECT_EQ(ToBFOutOfClass->getLexicalDeclContext(), ToTU);
+
+  // Check that the redecl chain is intact.
+  EXPECT_EQ(ToBFOutOfClass->getPreviousDecl(), ToBFInClass);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDefInSeparateCode) {
+  auto CodeTU0 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  )";
+  auto CodeTU1 =
+  R"(
+  struct B { virtual void f(); };
+  struct D:B { void f(); };
+  void B::f(){}
+  void D::f(){}
+  void foo(B , D ) { b.f(); d.f(); }
+  )";
+
+  auto BFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B";
+  auto BFPIsDefP = cxxMethodDecl(
+  hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), 

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik marked 17 inline comments as done.
shafik added a comment.

@martong I don't follow the discussion on `VisitParmVarDecl` an what seems like 
an alternate fix. Can you elaborate?




Comment at: lib/AST/ASTImporter.cpp:3042
 
+  if (FoundByLookup) {
+if (auto *MD = dyn_cast(FoundByLookup)) {

martong wrote:
> It is not trivial why we add this block to the code, so I think a comment 
> would be really appreciated here.
> I was thinking about something like this:
> ```
> +  // We do not allow more than one in-class prototype of a function.  This is
> +  // because AST clients like VTableBuilder asserts on this.  VTableBuilder
> +  // assumes that only one function can override a function. Building a 
> redecl
> +  // chain would result there are more than one function which override the
> +  // base function (even if they are part of the same redecl chain inside the
> +  // derived class.)
> ```
Just for clarification, from a C++ perspective, I would say "in-class 
declaration" rather than "in-class prototype" and therefore `VTableBuilder` 
assumes only one in class declaration and building a redecal chain would result 
in more than one in-class declaration being present. 

Does that makes sense?



Comment at: unittests/AST/ASTImporterTest.cpp:2344
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDefTwoTUs) {
+  auto CodeTU0 =

balazske wrote:
> The previous tests use two TUs too so the name for this is not exact (here 
> the code is different, maybe use 
> `ImportOverriddenMethodTwiceOutOfClassDefInSeparateCode`?). This test does 
> roughly the same as `ImportOverriddenMethodTwiceDefinitionFirst` but with the 
> definition last and out-of-class version. (Name can be 
> `ImportOverriddenMethodTwiceOutOfClassDefLast` too.) I am not sure why is the 
> `foo` used here (`B::F` can be imported instead).
I originally thought I did not need `foo` either but I could get it to import 
the definition of `B::f()` any other way.

I also saw similar behavior with `clang-import-test` where I need an expression 
that included something like `foo()` in order to obtain the out-of-line 
definition. It is not obvious to me why.

So in this case if I switch to using `BFP` instead of `FooDef` then this fails:

auto *ToBFOutOfClass = FirstDeclMatcher().match(
  ToTU, cxxMethodDecl(hasName("f"), isDefinition()));




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

https://reviews.llvm.org/D56936



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56581: [ASTImporter] Set the described template if not set

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a subscriber: aprantl.
shafik added a comment.

I don't believe it is directly related to modules but that it just triggers the 
issue because it may be bringing in more. You can find a description here 
https://clang.llvm.org/docs/CommandGuide/clang.html#cmdoption-g and @aprantl 
pointed me to this talk if you want to even more 
http://llvm.org/devmtg/2015-10/#talk19


Repository:
  rC Clang

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

https://reviews.llvm.org/D56581



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56581: [ASTImporter] Set the described template if not set

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

@martong have you had a chance to look at this some more? We ran into another 
problem that this fixes partially. Can I help in any way?


Repository:
  rC Clang

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

https://reviews.llvm.org/D56581



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 {
+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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC357940: [ASTImporter] Call to HandleNameConflict in 
VisitEnumDecl mistakeningly using… (authored by shafik, committed by ).
Herald added a project: clang.

Repository:
  rC Clang

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

https://reviews.llvm.org/D59665

Files:
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2441,7 +2441,7 @@
 }
 
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),
  ConflictingDecls.size());
   if (!Name)


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2441,7 +2441,7 @@
 }
 
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),
  ConflictingDecls.size());
   if (!Name)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I ran `check-lldb` and I hit one regression in `TestFormatters.py`, part of 
what I am seeing is as follows:

  AssertionError: False is not True : FileCheck'ing result of `expression 
--show-types -- *(new foo(47))`
  Error output:
  error: no matching constructor for initialization of 'foo'
  candidate constructor (the implicit copy constructor) not viable: no known 
conversion from 'int' to 'const foo' for 1st argument
  candidate constructor (the implicit move constructor) not viable: no known 
conversion from 'int' to 'foo' for 1st argument




Comment at: clang/lib/AST/ASTImporter.cpp:1818
+  // The class will have DefinitionData, but no members.  Then,
+  // importDefinition is called from LLDB, which tries to get the members, se
+  // when we get here, the class already has the DefinitionData set, so we must

se -> so?



Comment at: lldb/source/Symbol/ClangASTImporter.cpp:922
+
+  Log *log_ast(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_AST));
+  if (log_ast) {

I am going to say that the logging change is an excellent additions and stands 
alone from this change. Although I realize the test depends on this new 
feature. It makes sense to add the logging in a separate PR.

I also say this b/c I found a regression and it would be nice to get the 
logging in while we resolve the regression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I don't see any regressions but I am a little uncomfortable since there are no 
tests. So I would feel better if this was split into three parts: Namespaces, 
Enums and Templates.

Are there small test programs that fail due to the missing data? We can add 
them as regression tests.




Comment at: lib/AST/ASTImporter.cpp:6129
 if (Error Err =
-ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
+ImportTemplateArgumentListInfo(E->getLAngleLoc(), 
E->getRAngleLoc(),
+   E->template_arguments(), ToTAInfo))

Curious why you decided to add the new arguments to the front as opposed to the 
end?



Comment at: lib/AST/ASTImporter.cpp:7150
+  auto Imp = importSeq(E->getQualifierLoc(), E->getTemplateKeywordLoc(),
+   E->getDeclName(), E->getNameInfo().getLoc(),
+   E->getLAngleLoc(), E->getRAngleLoc());

Can you explain why `E->getNameInfo().getLoc()` is more correct than 
`E->getExprLoc()`?



Comment at: lib/AST/ASTImporter.cpp:7225
 
-  if (E->hasExplicitTemplateArgs() && E->getTemplateKeywordLoc().isValid()) {
+  if (E->hasExplicitTemplateArgs()) {
 TemplateArgumentListInfo ToTAInfo;

We still want to import a few lines down even if 
`!E->getTemplateKeywordLoc().isValid()`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik requested changes to this revision.
shafik added a comment.
This revision now requires changes to proceed.

Actually I was mistaken, we can see the difference for `EnumDecl` and 
`ClassTemplateSpecializationDecl` as well.

For `EnumDecl` before:

  EnumDecl 0x7fd0ae884800  col:6 referenced B

After:

  EnumDecl 0x7fa703882600  line:2:6 referenced B

For `ClassTemplateSpecializationDecl` before:

  ClassTemplateSpecializationDecl 0x7f8c338846a0  col:7 
class A definition

after:

  ClassTemplateSpecializationDecl 0x7fca150832a0  line:5:7 
class A definition

So once we have tests similar to D61140  I 
would feel good about this change. Especially since it is not clear when the 
other changes will be in and tests are important to guard against regressions.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

So an alternative to testing could be matching the AST output from a 
`clang-import-test` like we did here:

https://reviews.llvm.org/D61140

although it unfortunately looks like only only the AST dump of `NamespaceDecl` 
output the `SourceLocation` you are fixing, see it on godbolt 
 when I try a `clang-import-test` it looks like 
it is missing that `SourceLocation`:

  |-NamespaceDecl 0x7f8c1f884750 > col:11 

and with your patch applied I see the `SourceLocation`:

  |-NamespaceDecl 0x7fe3e6882f50  line:1:11 A


Repository:
  rC Clang

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

https://reviews.llvm.org/D60499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I think the AST dump for `EnumDecl` and `ClassTemplateSpecializationDecl` 
should be dumping the missing `SourceLocations` but I am having trouble 
tracking down where that should be done.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Done with first round.




Comment at: lib/AST/ASTImporter.cpp:1643
+if (!ImportedOrErr) {
+  // For RecordDecls, failed import of a field will break the layout of the
+  // structure. Handle it as an error.

What cases are failed import of a field ok? Is that because we will get the 
field later on by another path?



Comment at: lib/AST/ASTImporter.cpp:1655
+  // Reorder declarations in RecordDecls because they may have another
+  // order. Keeping field order is vitable because it determines structure
+  // layout.

vitable?



Comment at: lib/AST/ASTImporter.cpp:1663
+
+
+  // NOTE: Here and below, we cannot call field_begin() method and its callers

Maybe a comment here explaining the purpose of the loops below. IIUC removing 
fields since they may have been imported in the wrong order and then adding 
them back in what should be the correct order now.



Comment at: lib/AST/ASTImporter.cpp:1674
+  Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+  assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
+  ToRD->removeDecl(ToD);

Are we sure `ToD` is never a `nullptr` b/c you are unconditionally derefenecing 
it here.



Comment at: lib/AST/ASTImporter.cpp:1679
+
+  if (!ToRD->hasExternalLexicalStorage())
+assert(ToRD->field_empty());

Can you add a comment explaining why if the Decl has ExternalLexicalStorage the 
fields might not be empty even though we just removed them?


Repository:
  rC Clang

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

https://reviews.llvm.org/D44100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62312: [ASTImporter] Added visibility context check for CXXRecordDecl.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Minor comments, I am going to run `check-lldb` now.




Comment at: unittests/AST/ASTImporterVisibilityTest.cpp:34
 };
+struct GetClassPattern {
+  using DeclTy = CXXRecordDecl;

`GetCXXRecordPattern` feels more consistent.



Comment at: unittests/AST/ASTImporterVisibilityTest.cpp:49
+// CXXRecordDecl:
+auto *ExternC = "class X;";
+auto *AnonC = "namespace { class X; }";

`const`? It is not consistent w/ the previous declarations. 



Comment at: unittests/AST/ASTImporterVisibilityTest.cpp:50
+auto *ExternC = "class X;";
+auto *AnonC = "namespace { class X; }";
 

`const`? It is not consistent w/ the previous declarations. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D62312



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62312: [ASTImporter] Added visibility context check for CXXRecordDecl.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D62312



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/trunk/unittests/AST/ASTImporterTest.cpp:4054
+}
   }
 

aganea wrote:
> Fixes
> ```
> [2097/2979] Building CXX object 
> tools/clang/unittests/Tooling/CMakeFiles/ToolingTests.dir/LookupTest.cpp.o
> /mnt/f/svn/clang/unittests/Tooling/LookupTest.cpp: In lambda function:
> /mnt/f/svn/clang/unittests/Tooling/LookupTest.cpp:220:8: warning: suggest 
> explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
>  if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
> ^
> ```
You mixed up the error messages but I see what is going on.

So you may want to add a comment since it is not apparent that what is going on 
is due the `EXPECT_TRUE` macro eventually expanding to an `if..else` which is 
what is triggering the warning. Since someone may come by in the future and 
just remove the braces since it is not apparent why they are there.

Same below as well.


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

https://reviews.llvm.org/D61046



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61140: Copy Argument Passing Restrictions setting when importing a CXXRecordDecl definition

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: martong, teemperor, aprantl, a_sidorin.
Herald added a subscriber: rnkovacs.

For a `CXXRecordDecl` the `RecordDeclBits` are stored in the `DeclContext`. 
Currently when we import the definition of a `CXXRecordDecl` via the 
`ASTImporter` we do not copy over this data.

We had a LLDB expression parsing bug where we would set it to not pass in 
registers:

  setArgPassingRestrictions(clang::RecordDecl::APK_CannotPassInRegs);

but when imported this setting would be lost. So dumping the `CXXRecordDecl` 
before importing we would see:

  CXXRecordDecl 0x7faaba292e50 <>  struct Bounds 
definition
  |-DefinitionData standard_layout has_user_declared_ctor can_const_default_init
  ...

but after importing it would show the following:

  CXXRecordDecl 0x7ff286823c50 <>  struct Bounds 
definition
  |-DefinitionData pass_in_registers standard_layout has_user_declared_ctor 
can_const_default_init
 ^

There will be a separate LLDB PR that will have a test that covers this and 
introduces a related fix for LLDB.

Note, we did not copy over any other of the `RecordDeclBits` since we don't 
have tests for those. We know that copying over 
`LoadedFieldsFromExternalStorage` would be a error and that may be the case for 
others as well.


https://reviews.llvm.org/D61140

Files:
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1767,6 +1767,9 @@
 ToData.HasDeclaredCopyAssignmentWithConstParam
   = FromData.HasDeclaredCopyAssignmentWithConstParam;
 
+// Copy over the data stored in RecordDeclBits
+ToCXX->setArgPassingRestrictions(FromCXX->getArgPassingRestrictions());
+
 SmallVector Bases;
 for (const auto  : FromCXX->bases()) {
   ExpectedType TyOrErr = import(Base1.getType());


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1767,6 +1767,9 @@
 ToData.HasDeclaredCopyAssignmentWithConstParam
   = FromData.HasDeclaredCopyAssignmentWithConstParam;
 
+// Copy over the data stored in RecordDeclBits
+ToCXX->setArgPassingRestrictions(FromCXX->getArgPassingRestrictions());
+
 SmallVector Bases;
 for (const auto  : FromCXX->bases()) {
   ExpectedType TyOrErr = import(Base1.getType());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61140: Copy Argument Passing Restrictions setting when importing a CXXRecordDecl definition

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 196757.
shafik added a comment.

Added test


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

https://reviews.llvm.org/D61140

Files:
  lib/AST/ASTImporter.cpp
  test/Import/cxx-record-flags/Inputs/F.cpp
  test/Import/cxx-record-flags/test.cpp


Index: test/Import/cxx-record-flags/test.cpp
===
--- /dev/null
+++ test/Import/cxx-record-flags/test.cpp
@@ -0,0 +1,14 @@
+// RUN: clang-import-test -dump-ast -import %S/Inputs/F.cpp -expression %s | 
FileCheck %s
+
+// CHECK: FTrivial
+// CHECK: DefinitionData
+// CHECK-SAME: pass_in_registers
+
+// CHECK: FNonTrivial
+// CHECK-NOT: pass_in_registers
+// CHECK: DefaultConstructor
+
+void expr() {
+  FTrivial f1;
+  FNonTrivial f2;
+}
Index: test/Import/cxx-record-flags/Inputs/F.cpp
===
--- /dev/null
+++ test/Import/cxx-record-flags/Inputs/F.cpp
@@ -0,0 +1,9 @@
+class FTrivial {
+  int i;
+};
+
+struct FNonTrivial {
+  virtual ~FNonTrivial() = default;
+  int i;
+};
+
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1767,6 +1767,9 @@
 ToData.HasDeclaredCopyAssignmentWithConstParam
   = FromData.HasDeclaredCopyAssignmentWithConstParam;
 
+// Copy over the data stored in RecordDeclBits
+ToCXX->setArgPassingRestrictions(FromCXX->getArgPassingRestrictions());
+
 SmallVector Bases;
 for (const auto  : FromCXX->bases()) {
   ExpectedType TyOrErr = import(Base1.getType());


Index: test/Import/cxx-record-flags/test.cpp
===
--- /dev/null
+++ test/Import/cxx-record-flags/test.cpp
@@ -0,0 +1,14 @@
+// RUN: clang-import-test -dump-ast -import %S/Inputs/F.cpp -expression %s | FileCheck %s
+
+// CHECK: FTrivial
+// CHECK: DefinitionData
+// CHECK-SAME: pass_in_registers
+
+// CHECK: FNonTrivial
+// CHECK-NOT: pass_in_registers
+// CHECK: DefaultConstructor
+
+void expr() {
+  FTrivial f1;
+  FNonTrivial f2;
+}
Index: test/Import/cxx-record-flags/Inputs/F.cpp
===
--- /dev/null
+++ test/Import/cxx-record-flags/Inputs/F.cpp
@@ -0,0 +1,9 @@
+class FTrivial {
+  int i;
+};
+
+struct FNonTrivial {
+  virtual ~FNonTrivial() = default;
+  int i;
+};
+
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1767,6 +1767,9 @@
 ToData.HasDeclaredCopyAssignmentWithConstParam
   = FromData.HasDeclaredCopyAssignmentWithConstParam;
 
+// Copy over the data stored in RecordDeclBits
+ToCXX->setArgPassingRestrictions(FromCXX->getArgPassingRestrictions());
+
 SmallVector Bases;
 for (const auto  : FromCXX->bases()) {
   ExpectedType TyOrErr = import(Base1.getType());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66028: [ASTDump] Add is_anonymous to VisitCXXRecordDecl

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368591: [ASTDump] Add is_anonymous to VisitCXXRecordDecl 
(authored by shafik, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66028?vs=214436=214655#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66028

Files:
  cfe/trunk/lib/AST/TextNodeDumper.cpp
  cfe/trunk/test/AST/ast-dump-records.cpp


Index: cfe/trunk/lib/AST/TextNodeDumper.cpp
===
--- cfe/trunk/lib/AST/TextNodeDumper.cpp
+++ cfe/trunk/lib/AST/TextNodeDumper.cpp
@@ -1537,6 +1537,7 @@
 FLAG(isGenericLambda, generic);
 FLAG(isLambda, lambda);
 
+FLAG(isAnonymousStructOrUnion, is_anonymous);
 FLAG(canPassInRegisters, pass_in_registers);
 FLAG(isEmpty, empty);
 FLAG(isAggregate, aggregate);
Index: cfe/trunk/test/AST/ast-dump-records.cpp
===
--- cfe/trunk/test/AST/ast-dump-records.cpp
+++ cfe/trunk/test/AST/ast-dump-records.cpp
@@ -65,7 +65,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -87,7 +87,7 @@
 
   struct {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 struct definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -194,7 +194,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -217,7 +217,7 @@
 
   struct {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 struct definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit


Index: cfe/trunk/lib/AST/TextNodeDumper.cpp
===
--- cfe/trunk/lib/AST/TextNodeDumper.cpp
+++ cfe/trunk/lib/AST/TextNodeDumper.cpp
@@ -1537,6 +1537,7 @@
 FLAG(isGenericLambda, generic);
 FLAG(isLambda, lambda);
 
+FLAG(isAnonymousStructOrUnion, is_anonymous);
 FLAG(canPassInRegisters, pass_in_registers);
 FLAG(isEmpty, empty);
 FLAG(isAggregate, aggregate);
Index: cfe/trunk/test/AST/ast-dump-records.cpp
===
--- cfe/trunk/test/AST/ast-dump-records.cpp
+++ cfe/trunk/test/AST/ast-dump-records.cpp
@@ -65,7 +65,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -87,7 +87,7 @@
 
   struct {
 // CHECK-NEXT: 

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Just wanted to see if you were planning on landing this soon, the fix `Name` -> 
`SearchName` is probably an important one since we have seen several issues w/ 
bugs like that but I really would like to see more tests. Are you having issues 
coming up w/ tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65935: [ASTImporter] Import ctor initializers after setting flags.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

I was hoping to be able reproduce this in LLDB via an expression like this:

  expr testImportOfDelegateConstructor(10) == 10

but it does not. I am assuming the test ctu test case invokes the issues 
without the patch? I wonder why we don't also see it in as well.

Otherwise LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65935



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66348: [ASTImporter] Do not look up lambda classes

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I am not enthusiastic about this solution but I need to think about it some 
more.

We can see that p0624r2 
 added 
assignable lambdas:

  bool f1() {
  auto x = []{} = {}; auto x2 = x;
  
  return x == x2;
  }
  
  bool f2() {
  auto x = []{} = {};
  auto xb = []{} = {};
  
  return x == xb;
  }

see godbolt 

So I don't think this is a long-term solution, although I don't know what clang 
is doing to make this work yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66348



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66667: Debug Info: Support for DW_AT_export_symbols for anonymous structs

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 217271.
shafik marked an inline comment as done.
shafik added a comment.

Updating test to be more specific


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

https://reviews.llvm.org/D7

Files:
  lib/CodeGen/CGDebugInfo.cpp
  test/CodeGenCXX/debug-info-export_symbols.cpp


Index: test/CodeGenCXX/debug-info-export_symbols.cpp
===
--- /dev/null
+++ test/CodeGenCXX/debug-info-export_symbols.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -triple 
%itanium_abi_triple %s -o - | FileCheck %s
+
+// CHECK: [[SCOPE:![0-9]+]] = distinct !DICompositeType({{.*}}flags: 
DIFlagTypePassByValue
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, scope: 
[[SCOPE]]{{.*}}flags: DIFlagExportSymbols | DIFlagTypePassByValue
+struct A {
+ // Anonymous class exports its symbols into A
+ struct {
+ int y;
+ };
+} a;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3121,7 +3121,8 @@
 
   SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
 
-  // Explicitly record the calling convention for C++ records.
+  // Explicitly record the calling convention and export symbols for C++
+  // records.
   auto Flags = llvm::DINode::FlagZero;
   if (auto CXXRD = dyn_cast(RD)) {
 if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
@@ -3132,6 +3133,10 @@
 // Record if a C++ record is non-trivial type.
 if (!CXXRD->isTrivial())
   Flags |= llvm::DINode::FlagNonTrivial;
+
+// Record exports it symbols to the containing structure.
+if (CXXRD->isAnonymousStructOrUnion())
+Flags |= llvm::DINode::FlagExportSymbols;
   }
 
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(


Index: test/CodeGenCXX/debug-info-export_symbols.cpp
===
--- /dev/null
+++ test/CodeGenCXX/debug-info-export_symbols.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -triple %itanium_abi_triple %s -o - | FileCheck %s
+
+// CHECK: [[SCOPE:![0-9]+]] = distinct !DICompositeType({{.*}}flags: DIFlagTypePassByValue
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, scope: [[SCOPE]]{{.*}}flags: DIFlagExportSymbols | DIFlagTypePassByValue
+struct A {
+ // Anonymous class exports its symbols into A
+ struct {
+ int y;
+ };
+} a;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3121,7 +3121,8 @@
 
   SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
 
-  // Explicitly record the calling convention for C++ records.
+  // Explicitly record the calling convention and export symbols for C++
+  // records.
   auto Flags = llvm::DINode::FlagZero;
   if (auto CXXRD = dyn_cast(RD)) {
 if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
@@ -3132,6 +3133,10 @@
 // Record if a C++ record is non-trivial type.
 if (!CXXRD->isTrivial())
   Flags |= llvm::DINode::FlagNonTrivial;
+
+// Record exports it symbols to the containing structure.
+if (CXXRD->isAnonymousStructOrUnion())
+Flags |= llvm::DINode::FlagExportSymbols;
   }
 
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

Other than my two comment this LGTM




Comment at: clang/lib/AST/ASTImporter.cpp:3452
 << FoundField->getType();
-
-  return make_error(ImportError::NameConflict);
+  ConflictingDecls.push_back(FoundField);
 }

I am a little concerned about this change. I am wondering if this was 
previously handled differently as a band-aid for some other issues that only 
showed up in rare cases when they iterated over all the results but not when 
they errored out on the first. 

Could we make the changes to `VisitFieldDecl` a separate PR so it is easier to 
revert later on if we do find this causes a regression we are not currently 
covering?



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5615
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, Typedef) {
+  Decl *ToTU = getToTuDecl(

Since the "Liberal" strategy should be the current status quo, I think it might 
make sense to have a first PR that just has the `*LiberalStrategy` tests to 
verify that indeed this is the current behavior as we expect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66667: Debug Info: Support for DW_AT_export_symbols for anonymous structs

[Shafik Yaghmour via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG528f5da6d862: Debug Info: Support for DW_AT_export_symbols 
for anonymous structs (authored by shafik).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D7?vs=217457=217482#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D7

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/debug-info-export_symbols.cpp


Index: clang/test/CodeGenCXX/debug-info-export_symbols.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/debug-info-export_symbols.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -triple 
%itanium_abi_triple %s -o - | FileCheck %s
+
+// CHECK: [[SCOPE:![0-9]+]] = distinct !DICompositeType({{.*}}flags: 
DIFlagTypePassByValue
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, scope: [[SCOPE]]
+// CHECK-SAME:  DIFlagExportSymbols | 
DIFlagTypePassByValue
+struct A {
+ // Anonymous class exports its symbols into A
+ struct {
+ int y;
+ };
+} a;
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3121,7 +3121,8 @@
 
   SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
 
-  // Explicitly record the calling convention for C++ records.
+  // Explicitly record the calling convention and export symbols for C++
+  // records.
   auto Flags = llvm::DINode::FlagZero;
   if (auto CXXRD = dyn_cast(RD)) {
 if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
@@ -3132,6 +3133,10 @@
 // Record if a C++ record is non-trivial type.
 if (!CXXRD->isTrivial())
   Flags |= llvm::DINode::FlagNonTrivial;
+
+// Record exports it symbols to the containing structure.
+if (CXXRD->isAnonymousStructOrUnion())
+Flags |= llvm::DINode::FlagExportSymbols;
   }
 
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(


Index: clang/test/CodeGenCXX/debug-info-export_symbols.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/debug-info-export_symbols.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -triple %itanium_abi_triple %s -o - | FileCheck %s
+
+// CHECK: [[SCOPE:![0-9]+]] = distinct !DICompositeType({{.*}}flags: DIFlagTypePassByValue
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, scope: [[SCOPE]]
+// CHECK-SAME:  DIFlagExportSymbols | DIFlagTypePassByValue
+struct A {
+ // Anonymous class exports its symbols into A
+ struct {
+ int y;
+ };
+} a;
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3121,7 +3121,8 @@
 
   SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
 
-  // Explicitly record the calling convention for C++ records.
+  // Explicitly record the calling convention and export symbols for C++
+  // records.
   auto Flags = llvm::DINode::FlagZero;
   if (auto CXXRD = dyn_cast(RD)) {
 if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
@@ -3132,6 +3133,10 @@
 // Record if a C++ record is non-trivial type.
 if (!CXXRD->isTrivial())
   Flags |= llvm::DINode::FlagNonTrivial;
+
+// Record exports it symbols to the containing structure.
+if (CXXRD->isAnonymousStructOrUnion())
+Flags |= llvm::DINode::FlagExportSymbols;
   }
 
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64480: [ASTImporter] Added visibility context check for TypedefNameDecl.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: lib/AST/ASTImporter.cpp:949
+return Importer.GetFromTU(Found) == From->getTranslationUnitDecl();
+  return From->isInAnonymousNamespace() == Found->isInAnonymousNamespace();
+}

I am not sure what case this covers? Can you elaborate? I see the case the 
condition above is catching.



Comment at: unittests/AST/ASTImporterVisibilityTest.cpp:348
+::testing::Values(
+std::make_tuple(ExternTypedef, ExternTypedef, ExpectLink),
+std::make_tuple(ExternTypedef, AnonTypedef, ExpectNotLink),

Perhaps `ExpectLink` would be better as `ExpectLinkedDeclChain` and the same 
for `ExpectNotLink`.

It was actually confusing at first because link is an overload term.



Comment at: unittests/AST/ASTImporterVisibilityTest.cpp:352
+std::make_tuple(AnonTypedef, AnonTypedef, ExpectNotLink))), );
+INSTANTIATE_TEST_CASE_P(
+ParameterizedTests, ImportTypeAliasVisibility,

We need another set of tests for typedefs and using e.g. `ExternTypedef` and 
`ExternUsing` since they are equivalent. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D64480



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66999: [ASTImporter][NFC] Add comments to code where we cannot use HandleNameConflict

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:3454
 << FoundField->getType();
-
+  // This case is not handled with HandleNameConflict, because by the time
+  // when we reach here, the enclosing class is already imported. If there

Do we have an example that hits this case?



Comment at: clang/lib/AST/ASTImporter.cpp:5262
+  // template.
+  // FIXME Perhaps return with a different error?
   return make_error(ImportError::NameConflict);

I believe this is ill-formed no diagnostic required. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66999



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64480: [ASTImporter] Added visibility context check for TypedefNameDecl.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

It is worth noting that:

  typedef int T;
  typedef int T;

is not valid C99 see godbolt 


Repository:
  rC Clang

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

https://reviews.llvm.org/D64480



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64480: [ASTImporter] Added visibility context check for TypedefNameDecl.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:343
+::testing::Values(
+std::make_tuple(ExternTypedef, ExternTypedef, ExpectLink),
+std::make_tuple(ExternTypedef, AnonTypedef, ExpectNotLink),

I don't have any objections to changing the names `ExpectLink` and 
`ExpectNotLink` in a subsequent patch but we should update then b/c they are 
confusing (because they are overloaded terms) and it takes some time to work 
backwards to understand the intent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64480



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64480: [ASTImporter] Added visibility context check for TypedefNameDecl.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D64480#1654354 , @balazske wrote:

> In D64480#1653629 , @shafik wrote:
>
> > It is worth noting that:
> >
> >   typedef int T;
> >   typedef int T;
> >
> >
> > is not valid C99 see godbolt 
>
>
> Should we handle this case? This can be special for C99 only when the 
> declarations must be merged instead of linked. Probably this does not cause 
> functional problems if we leave it as is.


I don't think it is critical to handle this case but I was surprised when I 
learned this, it is an edge case and it would be good to note this perhaps as a 
comment in the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64480



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66348: [ASTImporter] Do not look up lambda classes

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

I was concerned about how this would affect LLDB but after thinking about it I 
realized that in the DWARF we will just end up with one `DW_TAG_class_type`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66348



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66933: [ASTImporter] Propagate errors during import of overridden methods.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM but I agree w/ Gabor's comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66933



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66667: Debug Info: Support for DW_AT_export_symbols for anonymous structs

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 217457.
shafik marked an inline comment as done.
shafik added a comment.

Simplifying test.


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

https://reviews.llvm.org/D7

Files:
  lib/CodeGen/CGDebugInfo.cpp
  test/CodeGenCXX/debug-info-export_symbols.cpp


Index: test/CodeGenCXX/debug-info-export_symbols.cpp
===
--- /dev/null
+++ test/CodeGenCXX/debug-info-export_symbols.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -triple 
%itanium_abi_triple %s -o - | FileCheck %s
+
+// CHECK: [[SCOPE:![0-9]+]] = distinct !DICompositeType({{.*}}flags: 
DIFlagTypePassByValue
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, scope: [[SCOPE]]
+// CHECK-SAME:  DIFlagExportSymbols | 
DIFlagTypePassByValue
+struct A {
+ // Anonymous class exports its symbols into A
+ struct {
+ int y;
+ };
+} a;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3121,7 +3121,8 @@
 
   SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
 
-  // Explicitly record the calling convention for C++ records.
+  // Explicitly record the calling convention and export symbols for C++
+  // records.
   auto Flags = llvm::DINode::FlagZero;
   if (auto CXXRD = dyn_cast(RD)) {
 if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
@@ -3132,6 +3133,10 @@
 // Record if a C++ record is non-trivial type.
 if (!CXXRD->isTrivial())
   Flags |= llvm::DINode::FlagNonTrivial;
+
+// Record exports it symbols to the containing structure.
+if (CXXRD->isAnonymousStructOrUnion())
+Flags |= llvm::DINode::FlagExportSymbols;
   }
 
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(


Index: test/CodeGenCXX/debug-info-export_symbols.cpp
===
--- /dev/null
+++ test/CodeGenCXX/debug-info-export_symbols.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -triple %itanium_abi_triple %s -o - | FileCheck %s
+
+// CHECK: [[SCOPE:![0-9]+]] = distinct !DICompositeType({{.*}}flags: DIFlagTypePassByValue
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, scope: [[SCOPE]]
+// CHECK-SAME:  DIFlagExportSymbols | DIFlagTypePassByValue
+struct A {
+ // Anonymous class exports its symbols into A
+ struct {
+ int y;
+ };
+} a;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3121,7 +3121,8 @@
 
   SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
 
-  // Explicitly record the calling convention for C++ records.
+  // Explicitly record the calling convention and export symbols for C++
+  // records.
   auto Flags = llvm::DINode::FlagZero;
   if (auto CXXRD = dyn_cast(RD)) {
 if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
@@ -3132,6 +3133,10 @@
 // Record if a C++ record is non-trivial type.
 if (!CXXRD->isTrivial())
   Flags |= llvm::DINode::FlagNonTrivial;
+
+// Record exports it symbols to the containing structure.
+if (CXXRD->isAnonymousStructOrUnion())
+Flags |= llvm::DINode::FlagExportSymbols;
   }
 
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66667: Debug Info: Support for DW_AT_export_symbols for anonymous structs

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added a reviewer: aprantl.
shafik added a project: debug-info.
shafik added a parent revision: D66605: Debug Info: Support for 
DW_AT_export_symbols for anonymous structs.

This implements the DWARF 5 feature described in:

http://dwarfstd.org/ShowIssue.php?issue=141212.1

To support recognizing anonymous structs:

  struct A { 
struct { // Anonymous struct 
int y;
};  
  } a;

This patch adds support in `CGDebugInfo::CreateLimitedType(...)` for this new 
flag and an accompanying test to verify this feature.


https://reviews.llvm.org/D7

Files:
  lib/CodeGen/CGDebugInfo.cpp
  test/CodeGenCXX/debug-info-export_symbols.cpp


Index: test/CodeGenCXX/debug-info-export_symbols.cpp
===
--- /dev/null
+++ test/CodeGenCXX/debug-info-export_symbols.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -triple 
%itanium_abi_triple %s -o - | FileCheck %s
+
+// CHECK-DAG: !DICompositeType({{.*}}flags: DIFlagExportSymbols | 
DIFlagTypePassByValue
+struct A {
+ // Anonymous class exports its symbols into A
+ struct {
+ int y;
+ };
+} a;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3121,7 +3121,8 @@
 
   SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
 
-  // Explicitly record the calling convention for C++ records.
+  // Explicitly record the calling convention and export symbols for C++
+  // records.
   auto Flags = llvm::DINode::FlagZero;
   if (auto CXXRD = dyn_cast(RD)) {
 if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
@@ -3132,6 +3133,10 @@
 // Record if a C++ record is non-trivial type.
 if (!CXXRD->isTrivial())
   Flags |= llvm::DINode::FlagNonTrivial;
+
+// Record exports it symbols to the containing structure.
+if (CXXRD->isAnonymousStructOrUnion())
+Flags |= llvm::DINode::FlagExportSymbols;
   }
 
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(


Index: test/CodeGenCXX/debug-info-export_symbols.cpp
===
--- /dev/null
+++ test/CodeGenCXX/debug-info-export_symbols.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -triple %itanium_abi_triple %s -o - | FileCheck %s
+
+// CHECK-DAG: !DICompositeType({{.*}}flags: DIFlagExportSymbols | DIFlagTypePassByValue
+struct A {
+ // Anonymous class exports its symbols into A
+ struct {
+ int y;
+ };
+} a;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3121,7 +3121,8 @@
 
   SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
 
-  // Explicitly record the calling convention for C++ records.
+  // Explicitly record the calling convention and export symbols for C++
+  // records.
   auto Flags = llvm::DINode::FlagZero;
   if (auto CXXRD = dyn_cast(RD)) {
 if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
@@ -3132,6 +3133,10 @@
 // Record if a C++ record is non-trivial type.
 if (!CXXRD->isTrivial())
   Flags |= llvm::DINode::FlagNonTrivial;
+
+// Record exports it symbols to the containing structure.
+if (CXXRD->isAnonymousStructOrUnion())
+Flags |= llvm::DINode::FlagExportSymbols;
   }
 
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67174: Rename of constants in ASTImporterVisibilityTest. NFC.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

Thank you! LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67174



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66951: [ASTImporter] Add comprehensive tests for ODR violation handling strategies

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/unittests/AST/ASTImporterODRStrategiesTest.cpp:151
+};
+
+template 

martong wrote:
> balazske wrote:
> > `FunctionTemplate` and `FunctionTemplateSpec` are missing?
> Yes, because `FunctionTemplates` overload with each other. So they are 
> imported always "liberally".
> 
> There is no point to liberally import conflicting 
> `FunctionTemplateSpecializations`.
> The only thing we can do in that case is to omit the conflicting declaration.
> And this is true in case of `ClassTemplateSpecialization`s too.
> 
> Perhaps we should remove `struct ClassTemplateSpec` as well from here (?).
> Because they are never going to be handled "liberally".
> 
> @shafik , what do you think about this?
What you say about `FunctionTemplateSpecializations` and 
`ClassTemplateSpecializations` seems to make sense, importing them liberally 
would require more than work in the importer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66951



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66951: [ASTImporter] Add comprehensive tests for ODR violation handling strategies

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/unittests/AST/ASTImporterODRStrategiesTest.cpp:118
+  template 
+  constexpr T X;
+  )";

Note this is not well-formed b/c it is not initialized, [see 
godbolt](https://godbolt.org/z/8xvFwh)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66951



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66951: [ASTImporter] Add comprehensive tests for ODR violation handling strategies

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/unittests/AST/ASTImporterODRStrategiesTest.cpp:118
+  template 
+  constexpr T X;
+  )";

shafik wrote:
> Note this is not well-formed b/c it is not initialized, [see 
> godbolt](https://godbolt.org/z/8xvFwh)
But it would be ok combined w/ a specialization:

```
template <>
constexpr int X = 0;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66951



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65573: Add User docs for ASTImporter

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Thank you for writing this up! I just have a few minor comments.




Comment at: clang/docs/LibASTImporter.rst:110
+
+Now we create the Importer and do the import:
+

Maybe helpful to link to the [Matching the Clang 
AST](https://clang.llvm.org/docs/LibASTMatchers.html) and [AST Matcher 
Reference](https://clang.llvm.org/docs/LibASTMatchersReference.html)



Comment at: clang/docs/LibASTImporter.rst:283
+However, there may be cases when both of the contexts have a definition for a 
given symbol.
+If these definitions differ then we have a name conflict, otherwise known as 
ODR (one definition rule) violation.
+Let's modify the previous tool we had written and try to import a 
``ClassTemplateSpecializationDecl`` with a conflicting definition:

Note ODR is a C++ concept C does not have ODR


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65573



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65577: [ASTImporter] Import default expression of param before creating the param.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:3859
   ToTypeSourceInfo, D->getStorageClass(),
   /*DefaultArg*/ nullptr))
 return ToParm;

This should be `DefaultArg` now?



Comment at: clang/test/Analysis/Inputs/ctu-other.cpp:135
+
+struct DefParmContext {
+  static const int I;

martong wrote:
> Perhaps we could write `Default` instead of `Def`.
+1 to this and the name suggestion below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66028: [ASTDump] Add is_anonymous to VisitCXXRecordDecl

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: aaron.ballman, aprantl.

Adding is_anonymous the ASTDump for `CXXRecordDecl`. This turned out to be 
useful when debugging some problems with how LLDB creates ASTs from DWARF.


https://reviews.llvm.org/D66028

Files:
  lib/AST/TextNodeDumper.cpp
  test/AST/ast-dump-records.cpp


Index: test/AST/ast-dump-records.cpp
===
--- test/AST/ast-dump-records.cpp
+++ test/AST/ast-dump-records.cpp
@@ -65,7 +65,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -87,7 +87,7 @@
 
   struct {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 struct definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -194,7 +194,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -217,7 +217,7 @@
 
   struct {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 struct definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate 
standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
Index: lib/AST/TextNodeDumper.cpp
===
--- lib/AST/TextNodeDumper.cpp
+++ lib/AST/TextNodeDumper.cpp
@@ -1537,6 +1537,7 @@
 FLAG(isGenericLambda, generic);
 FLAG(isLambda, lambda);
 
+FLAG(isAnonymousStructOrUnion, is_anonymous);
 FLAG(canPassInRegisters, pass_in_registers);
 FLAG(isEmpty, empty);
 FLAG(isAggregate, aggregate);


Index: test/AST/ast-dump-records.cpp
===
--- test/AST/ast-dump-records.cpp
+++ test/AST/ast-dump-records.cpp
@@ -65,7 +65,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -87,7 +87,7 @@
 
   struct {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 struct definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
+// CHECK-NEXT: DefinitionData is_anonymous pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
 // CHECK-NEXT: DefaultConstructor exists trivial needs_implicit
 // CHECK-NEXT: CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
 // CHECK-NEXT: MoveConstructor exists simple trivial needs_implicit
@@ -194,7 +194,7 @@
 
   union {
 // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}}  line:[[@LINE-1]]:3 union definition
-// CHECK-NEXT: DefinitionData pass_in_registers aggregate standard_layout 

[PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

This LGTM now but I will wait for @teemperor to take a look at it.




Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:620
 
 if (predicate(decl->getKind())) {
   if (log) {

I think a comment on the `predicate` would be helpful as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D44100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

First round of review.




Comment at: clang/lib/AST/ASTImporter.cpp:2632
+  ExpectedName NameOrErr = Importer.HandleNameConflict(
+  Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size());
+  if (!NameOrErr)

`Name` -> `SearchName`



Comment at: clang/unittests/AST/ASTImporterTest.cpp:2392
 
+struct ImportFunctionTemplates : ASTImporterOptionSpecificTestBase {};
+

What about tests for name conflicts for:

`NamespaceDecl` 
`TypedefNameDecl`
`TypeAliasTemplateDecl`
`EnumConstantDecl`
`RecordDecl`
`VarDecl`

Who were also modified above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60499



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits