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

labath added a comment. In D59692#1646990 , @martong wrote: > Jenkins is not green > http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/ > However, the newly failing test is `TestTargetCommand.py`, which seems to be > unrelated. That ought t

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

martong added a comment. Jenkins is not green http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/ However, the newly failing test is `TestTargetCommand.py`, which seems to be unrelated. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59692

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGf035b75d8f07: [ASTImporter] Fix name conflict handling with different strategies (authored by martong). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59692/n

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

martong updated this revision to Diff 217337. martong added a comment. - Apply clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59692/new/ https://reviews.llvm.org/D59692 Files: clang/include/clang/AST/ASTImporter.h clang/lib/AST/AST

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

martong updated this revision to Diff 217330. martong marked 2 inline comments as done. martong added a comment. - Revert changes in VisitFieldDecl and in VisitIndirectFieldDecl - Remove test suite ConflictingDeclsWithConservativeStrategy Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

martong marked 4 inline comments as done. martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:3452 << FoundField->getType(); - - return make_error(ImportError::NameConflict); + ConflictingDecls.push_back(FoundField); }

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

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

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

martong added a comment. @shafik I have updated the patch with ODR handling strategies as per our discusson. For the record I copy our discussion here: On Sat, Aug 24, 2019 at 12:50 AM Shafik Yaghmour wrote: > Apologies, I missed this earlier! > > On Wed, Aug 21, 2019 at 2:44 AM Gábor Márto

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

martong updated this revision to Diff 217135. martong added a comment. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. - Use resulting Name from HandleNameConflict if set - Add ODRHandling strategies - Refactor tests, to avoid some code repetition Repository: rG LLVM Git

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

martong marked an inline comment as done. martong added a comment. @shafik Ping 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

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

martong marked 2 inline comments as done. martong added inline comments. Comment at: clang/unittests/AST/ASTImporterTest.cpp:2392 +struct ImportFunctionTemplates : ASTImporterOptionSpecificTestBase {}; + shafik wrote: > What about tests for name conflicts for:

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

martong updated this revision to Diff 215579. martong added a comment. - Name -> SearchName - Add tests for conflicting declarations Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59692/new/ https://reviews.llvm.org/D59692 Files: clang/include/cl

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

martong marked an inline comment as done. martong added a comment. In D59692#1632138 , @shafik wrote: > 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

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

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

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

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

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

martong updated this revision to Diff 211312. martong added a comment. - Rebase to master Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59692/new/ https://reviews.llvm.org/D59692 Files: clang/include/clang/AST/ASTImporter.h clang/lib/AST/ASTIm

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

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59692/new/ https://reviews.llvm.org/D59692 ___

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

martong added a comment. Ping Repository: rC Clang 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/li

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

martong added a comment. Ping @a_sidorin @shafik Repository: rC Clang 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

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

martong updated this revision to Diff 194527. martong marked an inline comment as done. martong added a comment. - Put back the call to GetOriginalDecl Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59692/new/ https://reviews.llvm.org/D59692 Files: include/clan

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

martong marked 2 inline comments as done. martong added inline comments. Comment at: lib/AST/ASTImporter.cpp:2154 +return NameOrErr.takeError(); } } a_sidorin wrote: > Should we write `else Name = *NameOrError`? Atm, we implement the simplest stra

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

a_sidorin added a comment. Thanks for the fixes! I'd like to clarify one moment, however. Comment at: lib/AST/ASTImporter.cpp:2154 +return NameOrErr.takeError(); } } Should we write `else Name = *NameOrError`? Repository: rC Clang CHANGES S

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

martong updated this revision to Diff 192263. martong marked 2 inline comments as done. martong added a comment. - Add braces around else - Remove falsely duplicated push_back - Use Expected in HandleNameConflict Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59692

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

martong marked 5 inline comments as done. martong added inline comments. Comment at: lib/AST/ASTImporter.cpp:2260 ConflictingDecls.push_back(FoundDecl); } a_sidorin wrote: > Do we push the same decl twice? Uhh, thanks, that is rebase error I've made

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

a_sidorin added a comment. Hi Gabor, Thank you for addressing the problem! Comment at: lib/AST/ASTImporter.cpp:2256 return Importer.MapImported(D, FoundTypedef); -} -// FIXME Handle redecl chain. -break; +} else + Conflicti

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

martong created this revision. martong added reviewers: a_sidorin, shafik, teemperor. Herald added subscribers: cfe-commits, jdoerfert, gamesh411, Szelethus, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. Herald added a project: clang. martong added a parent revision: D55049: Changed every