teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.
I think the approach here makes sense. The only issue I see is how we get the
complete replacement type (see the inline comment).
There is the issue if having an artificial empty class in an expression is an
error, but we anyway already do this so I don't think this discussion should
block this patch.
Also the FindCompleteType refactoring should IMHO land as it's own NFC commit
just because it's such a huge part of the patch (I don't think that needs a
separate review, it LGTM).
================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp:891
+
+ if (auto *proxy = llvm::dyn_cast<ClangASTSource::ClangASTSourceProxy>(
+ getToContext().getExternalSource())) {
----------------
This check can really easily break. We should always be able to replace the
ExternalASTSource with some kind of multiplexer (like
MultiplexExternalSemaSource) without breaking functionality, so doing an RTTI
check for a specific class here will break that concept. For example activating
the import-std-module setting will make Clang add its own ASTReader source to
the AST, so the ClangASTSourceProxy is hidden in some Multiplex* class (that
contains ClangASTSourceProxy and the ASTReader) and this check will fail.
I think can simplified to this logic which doesn't require any RTTI support or
ClangASTSource refactoring (even though the FindCompleteType refactor is IMHO
something we should do even if we don't need it for this patch). I also removed
that we forcibly complete the class which I don't think is necessary:
```
lang=c++
if (td && md && md->IsForcefullyCompleted()) {
Expected<DeclContext *> dc_or_err = ImportContext(td->getDeclContext());
if (!dc_or_err)
return dc_or_err.takeError();
Expected<DeclarationName> dn_or_err = Import(td->getDeclName());
if (!dn_or_err)
return dn_or_err.takeError();
DeclContext *dc = *dc_or_err;
DeclarationName dn = *dn_or_err;
DeclContext::lookup_result lr = dc->lookup(dn);
if (lr.size()) {
clang::Decl *lookup_found = lr.front();
RegisterImportedDecl(From, lookup_found);
m_decls_to_ignore.insert(lookup_found);
return lookup_found;
}
}
```
The only required change for getting this to work is to get rid of the
`ImportInProgress` flag that completely disables all lookups when we copy a
type. I don't think there is a good reason for even having this flag as it's
anyway usually not correctly set when we do some kind of import, so I think
deleting it is fine.
================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/CxxModuleHandler.cpp:11
+#include "Plugins/ExpressionParser/Clang/ClangASTImporter.h"
+#include "Plugins/ExpressionParser/Clang/ClangASTMetadata.h"
#include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
----------------
I guess that's WIP code
================
Comment at: lldb/test/API/functionalities/limit-debug-info/Makefile:5
+
+ONE_CXXFLAGS := -flimit-debug-info
+ifdef STRIP_ONE
----------------
The in-tree limit-debug-info test already uses the LIMIT_DEBUG_INFO_FLAGS
variable instead of hardcoding the flag, so I think we might as well do it here
too.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81561/new/
https://reviews.llvm.org/D81561
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
