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

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

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

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL348923: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull (authored by martong, committed by ).

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

martong added a comment. @shafik > 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. I just have tested this patch on macOS and there seems to be no regression. Linux also seems to be without any

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

a_sidorin accepted this revision. a_sidorin added a comment. LGTM with a nit: the call to GetAlreadyImportedOrNull is not removed but moved into ImportDeclParts. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53755/new/ https://reviews.llvm.org/D53755

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

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

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

martong added a subscriber: teemperor. martong added a comment. Hi @shafik , Thank you for your review. I have removed the call of `getPrimaryContext`. Fortunately, we already have one patch for the `getPrimaryContext` related changes, made by @teemperor : https://reviews.llvm.org/D54898 . I

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

martong updated this revision to Diff 175639. martong marked an inline comment as done. martong added a comment. - Remove call of 'getPrimaryContext' Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53755/new/ https://reviews.llvm.org/D53755 Files:

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

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

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

martong marked 2 inline comments as done. martong added inline comments. Comment at: lib/AST/ExternalASTMerger.cpp:154 ToContainer->setHasExternalLexicalStorage(); - ToContainer->setMustBuildLookupTable(); +

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

a_sidorin added a comment. More constants are always welcome. Comment at: lib/AST/ExternalASTMerger.cpp:154 ToContainer->setHasExternalLexicalStorage(); - ToContainer->setMustBuildLookupTable(); + ToContainer->getPrimaryContext()->setMustBuildLookupTable();

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

martong added a subscriber: gamesh411. martong added a comment. @shafik , @gamesh411 I think when I used arcanist to update the patch it removed you as a reviewer and a subscriber. I am really sorry about this. Fortunately the Herald script added you guys back. And once you are here, this is a

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

martong marked an inline comment as done. martong added inline comments. Herald added a reviewer: shafik. Herald added a subscriber: gamesh411. Comment at: lib/AST/ASTImporter.cpp:7716 } } balazske wrote: > This can be simplified by removing brace

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

martong updated this revision to Diff 174897. martong marked an inline comment as done. martong removed a reviewer: shafik. martong removed a subscriber: gamesh411. martong added a comment. Herald added a reviewer: shafik. - Better style without braces Repository: rC Clang

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

balazske added inline comments. Comment at: lib/AST/ASTImporter.cpp:7716 } } This can be simplified by removing brace characters and removing `ToD`. Repository: rC Clang https://reviews.llvm.org/D53755

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

martong added a comment. This is about to make `GetAlreadyImportedOrNull` to be a const function, as it should be naturally. This is a query function which should not initiate other imports. Repository: rC Clang https://reviews.llvm.org/D53755

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

martong created this revision. Herald added subscribers: cfe-commits, Szelethus, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. a_sidorin Repository: rC Clang https://reviews.llvm.org/D53755 Files: include/clang/AST/ASTImporter.h lib/AST/ASTImporter.cpp