clayborg added a comment.
In http://reviews.llvm.org/D12658#243863, @paulherman wrote:
> I agree with most of your comments and will fix them.
>
> One thing I noticed is that Block::GetDeclContext() does not do the right
> thing (see inline comment).
Yep, we only ever needed the function decl context before this. We can rename
Block::GetDeclContext() to be Block::GetFunctionDeclContext(), and fix all
places that use it to use the new name. Then we modify Block::GetDeclContext()
to do what you want it to do.
Also note that you don't want any methods on lldb_private classes to be named
"GetClangXXX()" since these objects are abstracted from specific type systems,
everything has to be generic and abstract.
> About creating a CompilerDecl class, I agree with you. I used
> CompilerDeclContext since it was just a void* and it was convenient. Do you
> think that the FindVariable method should be in ClangASTContext since there
> is the need to actually interpret the CompilerDeclContext according to clang
> rules?
There will need to be a ClangASTContext::DeclContextXXXX call made, but this
will need to be a TypeSystem virtual function that is overridden by
ClangASTContext since it inherits from lldb_private::TypeSystem, but we should
have an abstract call on CompilerDeclContext that initiates the search
(CompilerDeclContext::FindVariable(...)), which in turn calls the
"TypeSystem::DeclContextFindVariable(void *)". This then lets you get back to
the clang specific ways of finding things.
We might want to go a bit more generic and make a function like:
class CompilerDeclContext
{
CompilerDeclList FindDecls(const char *name);
}
Since we actually want to start returning more than 1 thing for a search by
name and there might be a variable named "a" and a function named "a" in that
decl context? Or two overloads of a function "a" in the decl context. So the
question is: do we try and search for CompilerDecl objects in a
CompilerDeclContext so we can find all things whose name is "a", or do we stick
with more targeted things like "CompilerDeclContext::FindVariable(...)". If we
go the FindVariable route, we can probably skip making the new CompilerDecl and
CompilerDeclList classes and just return lldb::VariableSP or use a
lldb_private::VariableList class.
> Regarding ParseImportedNamespace, it also deals with "using NS::var"
> directives. Is there a method similar to FindNamespace that I can use?
We will need to add this to the CompilerDeclContext, TypeSystem and to the
SymbolVendor/SymbolFile interfaces, all in abstract ways.
> In SymbolFileDWARF::GetDeclContextDIEContainingDIE, is there a reason why
> DW_TAG_subprogram, DW_TAG_lexical_block, etc are not considered DeclContexts?
They currently are expecting it to ignore these things. The question is if that
is correct or not. We will need to look at who is asking. At the very least we
can add a bool that says "include_functions_blocks" and then include them in
the response if true if there are too many places that are using this
incorrectly. There are only 3 places that call this function so it wouldn't be
hard to fix them up to do the right thing.
The other thing Sean Callanan and Jim Ingham wanted to pass along is we want
lookups to happen in via the clang::ExternalASTSource function callbacks like:
namespace clang {
class ExternalASTSource : public RefCountedBase<ExternalASTSource> {
virtual bool
FindExternalVisibleDeclsByName(const DeclContext *DC, DeclarationName
Name);
}
}
I didn't look too closely at how your lookups were done, I mostly looked at the
overall CompilerDeclContext and DWARF parsing aspect, but that is their
preference. These lookups are always very targeted and say "inside the decl
context 'DC', find "Name".
http://reviews.llvm.org/D12658
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
