labath added a comment.

It looks like the Type interface does not expose the static members (I guess it 
was never meant to do that), although, the underlying clang type obviously does 
contain them. After more consideration, I think parsing these straight from 
DWARF is fine, and consistent with how we parse other variables. I am wondering 
though if this should share the implementation with the existing parsing code 
(like your first patch did).

On a higher level, I am also worried about what this (the fact that we have to 
do this kind of a search) says about the quality/usefulness of the debug_names 
index. Overall, I seems to me that _not_ including these member variables in 
the index is the right call -- they would bloat the index (you'd have to put 
one of these next to every type declaration), and there are (as we've seen) 
other ways to find them.

I am not sure what to do about namespace-level constexpr variables, though... 
Currently, clang seems to put them into the index, even though that's not 
specified by standard. My guess is that this is by accident (it just inserts 
all global vars, without checking the formal inclusion rules). However, the 
question is what that means. If we were to consider this a bug and "fix" clang 
to stop emitting them, then we'd need to find a way to locate them in lldb. And 
doing this for namespaces won't be as easy/cheap as it is for classes. Maybe it 
doesn't have to block this patch (as I said, I think this part is ok), but I 
think it would be useful to discuss this with the people responsible for the 
generation of the index in clang and the wider dwarf community. (Unfortunately, 
I don't have the bandwidth to drive that dicussion :( ).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92643/new/

https://reviews.llvm.org/D92643

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to