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

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

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

a_sidorin accepted this revision. a_sidorin added a comment. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59665/new/ https://reviews.llvm.org/D59665 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

martong added a comment. In D59665#1446764 , @a_sidorin wrote: > Hi Shafik, > Thank you for the explanation, it is much more clear to me now. But, as I > see, D59692 is going to discard the changes > this patch

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

a_sidorin added a comment. Hi Shafik, Thank you for the explanation, it is much more clear to me now. But, as I see, D59692 is going to discard the changes this patch introduces. @martong Gabor, do you expect the changes of this patch to be merged into yours,

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

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59665/new/ https://reviews.llvm.org/D59665 ___ cfe-commits mailing list

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

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 > >

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

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

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

martong added a comment. Looks good for me now, but we should make a similar change in VisitRecordDecl too. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59665/new/ https://reviews.llvm.org/D59665 ___ cfe-commits mailing list

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

martong added a comment. 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

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

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

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

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. >

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

martong added a comment. > 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. Just

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

martong added a comment. In D59665#1438911 , @a_sidorin wrote: > Hi Shafik, > > Honestly, I was always wondering what does HandleNameConflict actually do. > Its implementation in ASTImporter is trivial and I don't see any of its > overrides in LLDB code

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

martong added a comment. 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

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

a_sidorin added a subscriber: davide. a_sidorin added a comment. Hi Shafik, Honestly, I was always wondering what does HandleNameConflict actually do. Its implementation in ASTImporter is trivial and I don't see any of its overrides in LLDB code too. Why do we check its result to be non-empty

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

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:

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

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