labath wrote: > > > One thing I'd like to get feedback on is whether this should be looking > > > into base classes. The discussion on the original PR focuses on lexically > > > nested types, but going through base classes (following usual language > > > rules) is another form of nesting. Both the existing and the new > > > implementations don't do that, but I think it would be more natural to do > > > it. Of course, then the function can definitely return more than one > > > result... > > > > > > What you're describing here is in line with member lookup described in the > > Standard. While it might be a useful facility, it's at odds with the intent > > of `FindDirectNestedTypes()` to be a small building block for efficient AST > > traversal. For the use case I have for this function, any amount of lookup > > is going to regress the performance of the formatter, and subsequently > > responsiveness of the debugger. So if you're going to pursue this, I > > suggest exposing this as a new function. > > I don't expect the simple `DeclContext` lookup proposed here to be expensive. > What _is_ expensive from the data-formatters point of view is either > full-blown expression evaluation or a global search in DWARF for some type.
Agreed. This should be the same lookup that clang does every time is sees a `::`. Going through (potentially many) calls to FindTypes would be a different story. The reason I was asking is because I was not sure if this behavior was just a side-effect of how the original implementation worked, or if it was the intention all along. Judging by the response, it's the latter. > > Re. looking at base classes, don't have a strong opinion on it. There's some > precedent for looking at base classes already in `TypeSystemClang` (e.g., > `GetIndexOfChildWithName`). But as you say @labath, that would require an API > change That's makes sense. Even if it was not the intention, the API makes this the only reasonable implementation. I have no use for the base-class traversal (may change in the future). If there's is a need for it, we can add a different API. https://github.com/llvm/llvm-project/pull/89183 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
