https://github.com/zyn0217 approved this pull request.
Thank you for the quick fix! While I think the patch looks generally good to
me, it would be better to add some analysis of the bug.
That said, I took a closer look at it, and I think I probably have another
similar approach that doesn't involve extensive refactors. And here is my
analysis:
(Let's use the example from your test case)
```cpp
template <int N>
struct waldo {
using type = waldo<N - 1>::type::next;
};
```
So, we're somehow attempting to resolve the type/decl for `next`, which is of
`DependentNameType`. To achieve that, we want to inspect the type of its prefix
i.e. the `waldo<N - 1>::type`, which is another `NestedNameSpecifier` and thus
we shall find out the type of `waldo<N - 1>` first - which is of
`ClassTemplateSpecializationType`. Then, we would perform a name lookup for
`type` within the context thereof, which results in the solely `TypedefType`
we're currently in.
Then we move on and try to find out what the `next` is with the `TypedefType`
as its prefix. This would happen in `resolveDependentMember`, with `T` as
`TypedefType` and `Name` as `next`. We expect that prefix to be of a
`RecordType` so that we can perform a name lookup. That is
`resolveTypeToRecordDecl`, and the type alias would get unwrapped and become a
`DependentNameType` in the end.
And now we're successfully getting into an infinite loop: we want to know the
type of `type`, which ends up a type alias `type` that would boil down to
itself!
The PR resolves it by tracking every seen DependentNameType with a set. And
since we don't want to turn the stateless `HeuristicResolver` to something
stateful (e.g. adding extra data members), we have to add another layer to hide
our implementation - this is what you've done.
However, I think we could track these `DependentNameType`s by adding some extra
parameters. Specifically, we can pass through the `DNT` from
`resolveDependentNameType` to `resolveDependentMember` and
`resolveTypeToRecordDecl`, then inside `resolveTypeToRecordDecl`, we bail out
with null if the unwrapped `DependentNameType` from `T` is the same as the
`DNT` - I did an experiment and it turns out to be working.
That said, I'd like to leave the decision up to you - my approach doesn't seem
much clearer than yours!
(Please add some details to the commit message before merging :-)
https://github.com/llvm/llvm-project/pull/83542
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
