a_sidorin
Details
- Reviewers
a.sidorin shafik teemperor a_sidorin - Commits
- rGbe77a9846dff: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull
rC348923: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull
rL348923: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull
Diff Detail
- Repository
- rL LLVM
Event Timeline
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.
lib/AST/ASTImporter.cpp | ||
---|---|---|
7716 ↗ | (On Diff #171285) | This can be simplified by removing brace characters and removing ToD. |
lib/AST/ASTImporter.cpp | ||
---|---|---|
7716 ↗ | (On Diff #171285) | Good catch, changed it. |
@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 gentle ping. :)
More constants are always welcome.
lib/AST/ExternalASTMerger.cpp | ||
---|---|---|
154 ↗ | (On Diff #174897) | Do we have to do the same for NamespaceDecls? |
lib/AST/ExternalASTMerger.cpp | ||
---|---|---|
154 ↗ | (On Diff #174897) | Yes, I think so. 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<NamespaceDecl *>(this)->getOriginalNamespace(); Consequently, we should call setMustBuildLookupTable only on a NamespaceDecl if we know for sure that is a primary context. |
The rest of the changes look good.
lib/AST/ExternalASTMerger.cpp | ||
---|---|---|
147 ↗ | (On Diff #174897) | This change looks unrelated to the refactoring of GetAlreadyImportedOrNull() so should probably be in another PR |
154 ↗ | (On Diff #174897) | This change looks unrelated to the refactoring of GetAlreadyImportedOrNull() so should probably be in another PR |
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 have set that patch as the parent of this one (in the stack). Could you please take a look on that patch too?
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
LGTM with a nit: the call to GetAlreadyImportedOrNull is not removed but moved into ImportDeclParts.
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 regression. Very soon I am going to commit and I am going to monitor green.lab.llvm.org/green/view/LLDB/ for sign of any failure. I will revert in case of any failure asap.
Thanks for the review!