[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)

Luca Di sera via cfe-commits cfe-commits at lists.llvm.org
Sat Sep 30 01:06:24 PDT 2023


diseraluca wrote:

> > > > I gave it a quick try, and we would still end up with the same result in our codebase. But, generally, this would not probably be feasible for us as a solution.
> > > 
> > > 
> > > do you have an idea why? Can you show the diff of the changes you made? Is the void specialization not explicit?
> > 
> > 
> > I did a quick test with this:
> > ```
> > diff --git a/clang/lib/AST/QualTypeNames.cpp b/clang/lib/AST/QualTypeNames.cpp
> > index 26aaa96a1dc6..8b882eda83bb 100644
> > --- a/clang/lib/AST/QualTypeNames.cpp
> > +++ b/clang/lib/AST/QualTypeNames.cpp
> > @@ -287,8 +287,13 @@ static NestedNameSpecifier *createNestedNameSpecifierForScopeOf(
> >          //
> >          // Make the situation is 'useable' but looking a bit odd by
> >          // picking a random instance as the declaring context.
> > -        if (ClassTempl->spec_begin() != ClassTempl->spec_end()) {
> > -          Decl = *(ClassTempl->spec_begin());
> > +        auto specialization_iterator = std::find_if(
> > +            ClassTempl->spec_begin(), ClassTempl->spec_end(), [](auto a) {
> > +              return !a->isExplicitInstantiationOrSpecialization();
> > +            });
> > +
> > +        if (specialization_iterator != ClassTempl->spec_end()) {
> > +          Decl = *specialization_iterator;
> >            Outer = dyn_cast<NamedDecl>(Decl);
> >            OuterNS = dyn_cast<NamespaceDecl>(Decl);
> >          }
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > Do please let me know if this is incorrect or if I misunderstood your proposal.
> > We do have explicit references to the void specialization in the codebase so it would explain the previous choice. But I'm not aware of why it would be chosen as a non-explicit one too.
> > I can try to debug that on Monday if that can be useful. Albeit it might take some time.
> 
> You probably need [isExplicitSpecialization](https://clang.llvm.org/doxygen/classclang_1_1ClassTemplateSpecializationDecl.html#acd75ba25d34249d2e21ebbecbb2ef70e)()

Wouldn't `isExplicitSpecializationOrInstantiation` be a stronger guarantee than isExplicitSpecialization, such that it would exclude a superset of what is excluded by `isExplicitSpecialization`? If I did not misunderstand their source code.

Nonetheless, I've made a test run with `isExplicitSpecialization`, but in Qt's codebase we still get the same result.

I generally don't think we can depend on this kind of behavior, especially as it is far too difficult to control for the kind of consistency we want.

I initially missed the fact that the intention of this was to produce compilable code. While similar to our use-case it has slightly different requirements and I feel that may not be compatible with ours.

I think that is fine, and I think we don't need to force it to work for all use cases,  especially because I really don't want to break other people code. My assumption was that this might be an improvement for other use-cases too, but I failed to see the "give me code that compiles" use case.

The only thing that comes to mind would be to condition the behavior to be there or not. Similar to how we have `WighGlobalNsPrefix` we might have another boolean or similar, so that the default behavior remains the same but it can be conditioned to avoid this case.

But I don't think that is a particularly good solution, so I'm pretty happy with maintaining our own version too.

https://github.com/llvm/llvm-project/pull/67566


More information about the cfe-commits mailing list