[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
https://github.com/diseraluca closed https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
diseraluca wrote: Closing this one, as we intend to maintain our own local version. https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
diseraluca wrote: > > @vgvassilev If that is an acceptable interface for the LLVM interface then, > > yes, it would be perfect from our side, and I'm more than happy to update > > the PR in the next few days. > > Just to be sure that I understood your proposal. > > `getFullyQualified*` calls will accept a new parameter, a callable, that > > will be passed down the call chain up to > > `createNestedNameSpecifierForScopeOf(const ASTContext &, const Decl *, > > ...)` and will be called when the teplate case is encountered? Or are you > > thinking more of a callable that replaces the call to > > `createNestedNameSpecifierForScopeOf(const ASTContext &, const Decl *, > > ...)`? > > I hesitate. Maybe we can pass a custom "policy" option and incorporate your > code in there... We reiterate if the solution does not look good? >From our point of view any solution is acceptable. But do note that from our >side it is not so much about incorporating custom code as much as removing >that specific behavior at this point in time. Could you expand about the policy? Are you talking about a "printing policy" or a custom policy for `getFullyQualified`? https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
vgvassilev wrote: > @vgvassilev If that is an acceptable interface for the LLVM interface then, > yes, it would be perfect from our side, and I'm more than happy to update the > PR in the next few days. > > Just to be sure that I understood your proposal. > > `getFullyQualified*` calls will accept a new parameter, a callable, that will > be passed down the call chain up to > `createNestedNameSpecifierForScopeOf(const ASTContext &, const Decl *, ...)` > and will be called when the teplate case is encountered? Or are you thinking > more of a callable that replaces the call to > `createNestedNameSpecifierForScopeOf(const ASTContext &, const Decl *, ...)`? I hesitate. Maybe we can pass a custom "policy" option and incorporate your code in there... We reiterate if the solution does not look good? https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
diseraluca wrote: @vgvassilev If that is an acceptable interface for the LLVM interface then, yes, it would be perfect from our side, and I'm more than happy to update the PR in the next few days. Just to be sure that I understood your proposal. `getFullyQualified*` calls will accept a new parameter, a callable, that will be down the call chain up to `createNestedNameSpecifierForScopeOf(const ASTContext &, const Decl *, ...)` and will be called when the teplate case is encountered? Or are you thinking more of a callable that replaces the call to `createNestedNameSpecifierForScopeOf(const ASTContext &, const Decl *, ...)`? https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
vgvassilev wrote: @diseraluca, since this code touches a case where we do some best effort recovery, would it be possible to change that interface to take a lambda/callback function and specialize it in your end to cover the docs case? https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
diseraluca wrote:
> I still do not see why the proposed solution would not work. If it solves
> the minimal case that you provided in the description of this PR, I am afraid
> that there is some bit from the Qt setup that we do not understand.
I cannot for certain say if there is an issue or not with how the Qt codebase
is setup; but I think it is out of the scope of the task we are trying to solve
to change that based on the behavior of QDoc interoperations with Clang.
For what it is worth, I've been playing with a very small example to check a
few possibilities, and even excluding explicit specializations we still have
certain cases that are not stable.
Say in
```
#pragma once
template
class QList {
public:
using Foo = const T&;
void begin(Foo);
};
```
Adding something as `using Bar = QList;`, and this being the only code in
the translation unit, we still have the addition of such a line modifying our
output, qualifying the type of the first parameter of `begin` as
`QList::Foo`, even with your proposal.
Say we can stabilize that, so that it always qualify in the same way, say
`QList`.
If we are documenting, maybe, some method `QList::Foo QList::bar()` and
we show to the user the return type as `QList::Foo`, this is going to be
misleading,
albeit not as erroneous as the `QFuture` case, for our user.
Indeed, the return type of `bar` really depends on the instantiation of `QList`
that we are looking at and it would be extremely confusing for a user skimming
our documentation to think that it always returns a `QList::Foo`.
As a documentation generator we really want to look at the generic case for our
output.
Now, say we have a very stable output, albeit not adequate for our case, it
might simplify doing some postprocessing to qualify in the way we want, but at
that point we still need to maintain something relatively similar to
`getFullyQualified*`, thus why it isn't that much of a difference for our use
case to only have a more stable output.
There are some other cases that might be iffy for our use-case too, with this
behavior.
Thus why I think, albeit unfortunate, our use-cases just happen to be somewhat
in conflict with regards to the output of `getFullyQualified*`.
@vgvassilev Both me an my team are deeply thankful for your help. It was
really kind to try and find a solution for our case.
I will close this PR as I don't think it can be merged, as it would break other
people use cases.
I will leave it open 1 or 2 days more in case someone else wants to chime in
with something.
https://github.com/llvm/llvm-project/pull/67566
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
vgvassilev wrote: > 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. I wanted to filter out instantiations. > 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 think we could make the template specializations consistent so that you at least cure the symptom of your problem. > 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. Understood. That's a pretty strong requirement for this case and we cannot really make a compromise. The idea is to offer an API which gives the user how would they reach certain AST node from within the global scope if they had to type it. > 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. I would avoid doing that. In fact, I still do not see why the proposed solution would not work. If it solves the minimal case that you provided in the description of this PR, I am afraid that there is some bit from the Qt setup that we do not understand. https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
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(Decl);
> >OuterNS = dyn_cast(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
vgvassilev 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(Decl);
>OuterNS = dyn_cast(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)()
https://github.com/llvm/llvm-project/pull/67566
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
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(Decl);
OuterNS = dyn_cast(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.
https://github.com/llvm/llvm-project/pull/67566
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
vgvassilev 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? https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
diseraluca wrote: > @diseraluca, thanks for the thorough description. The point of these routines > is to produce code that compiles. I am not sure if we change `Foo::Bar` > with `Foo::Bar` it will compile. > > > Due to the way the current codebase is set up, the chosen specialization is > > `QFuture` so that, for example, `QFuture::constBegin()` would be > > shown to the user as returning a `QFuture::const_iterator`. > > Nonetheless, `QFuture` is the only specialization that cannot have a > > `const_iterator` and, as a consequence, doesn't have a `constBegin` method > > in the first place. > > I suspect that `QFuture` is an explicit specialization. The intent of > the code was to pick up an implicit specialization which follows much more > closely the template pattern. Would selecting the first non-explicit > instantiation fix your usecase? Thank you for your answer @vgvassilev. 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. Since that is the case, and we have somewhat conflicting requirements, I think the best path would be for me to indefinitely maintain a custom implementation of `getFullyQualified*`, leaving upstream as is. I would thus suggest to close this without merging. https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
vgvassilev wrote: @diseraluca, thanks for the thorough description. The point of these routines is to produce code that compiles. I am not sure if we change `Foo::Bar` with `Foo::Bar` it will compile. > Due to the way the current codebase is set up, the chosen specialization is > `QFuture` so that, for example, `QFuture::constBegin()` would be shown > to the user as returning a `QFuture::const_iterator`. Nonetheless, > `QFuture` is the only specialization that cannot have a > `const_iterator` and, as a consequence, doesn't have a `constBegin` method in > the first place. I suspect that `QFuture` is an explicit specialization. The intent of the code was to pick up an implicit specialization which follows much more closely the template pattern. Would selecting the first non-explicit instantiation fix your usecase? https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
mizvekov wrote: It's recommended to avoid introducing pure clang-format changes mixed with other non-style changes. The recommended way to avoid that is to run clang-format on the modified files on a separate commit, and then just merge that, no review required. https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
diseraluca wrote: Run clang-format on the patch. I made a bit of a mess as I haven't used the PR model in a very long time. Hopefully this is correct. https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
https://github.com/diseraluca edited https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Qualify non-dependent types of a class template with its declaration (PR #67566)
https://github.com/diseraluca edited https://github.com/llvm/llvm-project/pull/67566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
