Challenging "-Wctad-maybe-unsupported" for 9.0.0

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Challenging "-Wctad-maybe-unsupported" for 9.0.0

Kristóf Umann via cfe-dev
Hello all,

I'm currently testing 9.0.0 and I'm one of those strange people that uses -Weverything in production, as we only support a single clang-release at the same time. (Actually clang-cl, as it's Windows-only)
During the upgrade, I've noticed a new warning "-Wctad-maybe-unsupported".
At first, I was really happy seeing it and started updating some code that implicitly used it.
I'm even considering to propose an extension to our styleguide, as mentioned in the review of the warning: https://reviews.llvm.org/D56731
`Some style guides want to allow using CTAD only on types that "opt-in"`

However, after finishing my initial testing and applying some suppression and update locally, I'm changing my mind.
Over half of the updates I had to do were about `std::unique_lock`, `std::scope_guard` ... (and this without actively using this feature).
These classes, if I'm correct, are one of the selling point of CTAD and are prohibited thanks to this warning.

To circle back to title of the email, should -Wctad-maybe-unsupported actually be a compiler warning iso a clang-tidy check?
If so, would it make sense of having a white-list, like all `std::` classes?





_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Challenging "-Wctad-maybe-unsupported" for 9.0.0

Kristóf Umann via cfe-dev
On Sat, Aug 3, 2019 at 6:03 AM JVApen via cfe-dev <[hidden email]> wrote:
Hello all,

I'm currently testing 9.0.0 and I'm one of those strange people that uses -Weverything in production, as we only support a single clang-release at the same time. (Actually clang-cl, as it's Windows-only)
During the upgrade, I've noticed a new warning "-Wctad-maybe-unsupported".
At first, I was really happy seeing it and started updating some code that implicitly used it.
I'm even considering to propose an extension to our styleguide, as mentioned in the review of the warning: https://reviews.llvm.org/D56731
`Some style guides want to allow using CTAD only on types that "opt-in"`

However, after finishing my initial testing and applying some suppression and update locally, I'm changing my mind.
Over half of the updates I had to do were about `std::unique_lock`, `std::scope_guard` ... (and this without actively using this feature).
These classes, if I'm correct, are one of the selling point of CTAD and are prohibited thanks to this warning.

To circle back to title of the email, should -Wctad-maybe-unsupported actually be a compiler warning iso a clang-tidy check?

(You mean "should -Wctad-maybe-unsupported be a clang-tidy check instead of a compiler warning?", I assume.)
 
If so, would it make sense of having a white-list, like all `std::` classes?

FWIW, I continue to believe that it makes no sense to talk about the library author "opting in" to CTAD. Either the client programmer trusts CTAD (even if it runs the risk of deducing the wrong thing sometimes) and thus every set-of-missing-angle-brackets must be assumed to be intentional no matter what type it's on, or else the client programmer doesn't trust CTAD (because of the aforementioned risk), and thus every set-of-missing-angle-brackets must be assumed to be unintentional. The intentions of the library author don't matter at all in this equation.

The situation for me is exactly analogous to VLAs in C — including the shape of the workaround: "write an explicit call to malloc()" versus "write an explicit call to make_foo()".
https://reviews.llvm.org/D54565 implements a `-Wctad` warning analogous to `-Wvla`.

–Arthur

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Challenging "-Wctad-maybe-unsupported" for 9.0.0

Kristóf Umann via cfe-dev
I see it as very much like FTAD, which also has no explicit opt-in mechanism, and client programmers who omit template argument lists for function templates also run the risk of deducing the wrong thing sometimes (my upcoming CPPCon talk includes examples of this). In both cases, the sign that the client programmer intends to rely on deduction is that they omit the template argument list.

I'm not surprised you didn't find the warning to be very helpful, and I predict the percentage of spurious warnings will only increase as programmers become more comfortable with deduction for constructors templates like they are for other function templates.

On Sat, Aug 3, 2019 at 9:14 AM Arthur O'Dwyer via cfe-dev <[hidden email]> wrote:
On Sat, Aug 3, 2019 at 6:03 AM JVApen via cfe-dev <[hidden email]> wrote:
Hello all,

I'm currently testing 9.0.0 and I'm one of those strange people that uses -Weverything in production, as we only support a single clang-release at the same time. (Actually clang-cl, as it's Windows-only)
During the upgrade, I've noticed a new warning "-Wctad-maybe-unsupported".
At first, I was really happy seeing it and started updating some code that implicitly used it.
I'm even considering to propose an extension to our styleguide, as mentioned in the review of the warning: https://reviews.llvm.org/D56731
`Some style guides want to allow using CTAD only on types that "opt-in"`

However, after finishing my initial testing and applying some suppression and update locally, I'm changing my mind.
Over half of the updates I had to do were about `std::unique_lock`, `std::scope_guard` ... (and this without actively using this feature).
These classes, if I'm correct, are one of the selling point of CTAD and are prohibited thanks to this warning.

To circle back to title of the email, should -Wctad-maybe-unsupported actually be a compiler warning iso a clang-tidy check?

(You mean "should -Wctad-maybe-unsupported be a clang-tidy check instead of a compiler warning?", I assume.)
 
If so, would it make sense of having a white-list, like all `std::` classes?

FWIW, I continue to believe that it makes no sense to talk about the library author "opting in" to CTAD. Either the client programmer trusts CTAD (even if it runs the risk of deducing the wrong thing sometimes) and thus every set-of-missing-angle-brackets must be assumed to be intentional no matter what type it's on, or else the client programmer doesn't trust CTAD (because of the aforementioned risk), and thus every set-of-missing-angle-brackets must be assumed to be unintentional. The intentions of the library author don't matter at all in this equation.

The situation for me is exactly analogous to VLAs in C — including the shape of the workaround: "write an explicit call to malloc()" versus "write an explicit call to make_foo()".
https://reviews.llvm.org/D54565 implements a `-Wctad` warning analogous to `-Wvla`.

–Arthur
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Challenging "-Wctad-maybe-unsupported" for 9.0.0

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev


On Sat, Aug 3, 2019 at 10:14 AM Arthur O'Dwyer via cfe-dev <[hidden email]> wrote:
On Sat, Aug 3, 2019 at 6:03 AM JVApen via cfe-dev <[hidden email]> wrote:
Hello all,

I'm currently testing 9.0.0 and I'm one of those strange people that uses -Weverything in production, as we only support a single clang-release at the same time. (Actually clang-cl, as it's Windows-only)
During the upgrade, I've noticed a new warning "-Wctad-maybe-unsupported".
At first, I was really happy seeing it and started updating some code that implicitly used it.
I'm even considering to propose an extension to our styleguide, as mentioned in the review of the warning: https://reviews.llvm.org/D56731
`Some style guides want to allow using CTAD only on types that "opt-in"`

However, after finishing my initial testing and applying some suppression and update locally, I'm changing my mind.
Over half of the updates I had to do were about `std::unique_lock`, `std::scope_guard` ... (and this without actively using this feature).

What do you mean by "without actively using this feature"?

It is unfortunate that this warning will flag types designed to work with CTAD, but who don't need any explicit deduction guides.
Over time, I hope libraries that care about CTAD will address this by suppressing the warning manually; but that's not an ideal solution.

 
These classes, if I'm correct, are one of the selling point of CTAD and are prohibited thanks to this warning.

They're not meant to be triggered by this warning. It's just an oversight.

I'll update and audit libc++ to opt-in where needed. I'll also set up libc++'s test suite to build with this warning
enabled.

I'll try to get the fixed into 9.0.
 

To circle back to title of the email, should -Wctad-maybe-unsupported actually be a compiler warning iso a clang-tidy check?

(You mean "should -Wctad-maybe-unsupported be a clang-tidy check instead of a compiler warning?", I assume.)
 
If so, would it make sense of having a white-list, like all `std::` classes?

The diagnostic is no less valid for types in the standard library. The library author should audit the type to ensure it's CTAD safe, write tests, and then explicitly opt it in. This is
important for the correctness of client programs written using CTAD.


FWIW, I continue to believe that it makes no sense to talk about the library author "opting in" to CTAD.

Types have to be designed with CTAD in mine. Without explicit deduction guides CTAD often does the wrong thing.
Even otherwise unobservable qualities about how a constructor is lexically spelled in source can greatly impact CTAD.
Whether a template was designed to support CTAD is an important quality we should talk more about.

 
Either the client programmer trusts CTAD (even if it runs the risk of deducing the wrong thing sometimes) and thus every set-of-missing-angle-brackets must be assumed to be intentional no matter what type it's on, or else the client programmer doesn't trust CTAD (because of the aforementioned risk), and thus every set-of-missing-angle-brackets must be assumed to be unintentional.

This isn't a dichotomy.

To be clear, "-Wctad-maybe-unsupported" diagnoses only usages of CTAD on types without any explicit deduction guides.

  
The intentions of the library author don't matter at all in this equation.

Whether a type was designed with knowledge of CTAD in mind has a causal relationship with the correctness of usages of that type with CTAD.
As client programmer I don't want to use CTAD on types that haven't been changed since before CTAD was a feature.




The situation for me is exactly analogous to VLAs in C — including the shape of the workaround: "write an explicit call to malloc()" versus "write an explicit call to make_foo()".
https://reviews.llvm.org/D54565 implements a `-Wctad` warning analogous to `-Wvla`.

–Arthur
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Challenging "-Wctad-maybe-unsupported" for 9.0.0

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev


On Sat, Aug 3, 2019 at 10:54 AM Michael Spertus via cfe-dev <[hidden email]> wrote:
I see it as very much like FTAD, which also has no explicit opt-in mechanism, and client programmers who omit template argument lists for function templates also run the risk of deducing the wrong thing sometimes (my upcoming CPPCon talk includes examples of this). In both cases, the sign that the client programmer intends to rely on deduction is that they omit the template argument list.

I'm not surprised you didn't find the warning to be very helpful, and I predict the percentage of spurious warnings will only increase as programmers become more comfortable with deduction for constructors templates like they are for other function templates. 
 

As programmers start using this feature, I would be curious to learn what percentage of existing class templates work with CTAD out of the box.
Especially class templates with more than one template argument.


On Sat, Aug 3, 2019 at 9:14 AM Arthur O'Dwyer via cfe-dev <[hidden email]> wrote:
On Sat, Aug 3, 2019 at 6:03 AM JVApen via cfe-dev <[hidden email]> wrote:
Hello all,

I'm currently testing 9.0.0 and I'm one of those strange people that uses -Weverything in production, as we only support a single clang-release at the same time. (Actually clang-cl, as it's Windows-only)
During the upgrade, I've noticed a new warning "-Wctad-maybe-unsupported".
At first, I was really happy seeing it and started updating some code that implicitly used it.
I'm even considering to propose an extension to our styleguide, as mentioned in the review of the warning: https://reviews.llvm.org/D56731
`Some style guides want to allow using CTAD only on types that "opt-in"`

However, after finishing my initial testing and applying some suppression and update locally, I'm changing my mind.
Over half of the updates I had to do were about `std::unique_lock`, `std::scope_guard` ... (and this without actively using this feature).
These classes, if I'm correct, are one of the selling point of CTAD and are prohibited thanks to this warning.

To circle back to title of the email, should -Wctad-maybe-unsupported actually be a compiler warning iso a clang-tidy check?

(You mean "should -Wctad-maybe-unsupported be a clang-tidy check instead of a compiler warning?", I assume.)
 
If so, would it make sense of having a white-list, like all `std::` classes?

FWIW, I continue to believe that it makes no sense to talk about the library author "opting in" to CTAD. Either the client programmer trusts CTAD (even if it runs the risk of deducing the wrong thing sometimes) and thus every set-of-missing-angle-brackets must be assumed to be intentional no matter what type it's on, or else the client programmer doesn't trust CTAD (because of the aforementioned risk), and thus every set-of-missing-angle-brackets must be assumed to be unintentional. The intentions of the library author don't matter at all in this equation.

The situation for me is exactly analogous to VLAs in C — including the shape of the workaround: "write an explicit call to malloc()" versus "write an explicit call to make_foo()".
https://reviews.llvm.org/D54565 implements a `-Wctad` warning analogous to `-Wvla`.

–Arthur
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Challenging "-Wctad-maybe-unsupported" for 9.0.0

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev

On Sun, 4 Aug 2019 at 00:37, Eric Fiselier <[hidden email]> wrote:


On Sat, Aug 3, 2019 at 10:14 AM Arthur O'Dwyer via cfe-dev <[hidden email]> wrote:
On Sat, Aug 3, 2019 at 6:03 AM JVApen via cfe-dev <[hidden email]> wrote:
Hello all,

I'm currently testing 9.0.0 and I'm one of those strange people that uses -Weverything in production, as we only support a single clang-release at the same time. (Actually clang-cl, as it's Windows-only)
During the upgrade, I've noticed a new warning "-Wctad-maybe-unsupported".
At first, I was really happy seeing it and started updating some code that implicitly used it.
I'm even considering to propose an extension to our styleguide, as mentioned in the review of the warning: https://reviews.llvm.org/D56731
`Some style guides want to allow using CTAD only on types that "opt-in"`

However, after finishing my initial testing and applying some suppression and update locally, I'm changing my mind.
Over half of the updates I had to do were about `std::unique_lock`, `std::scope_guard` ... (and this without actively using this feature).

What do you mean by "without actively using this feature"?

We presented the use of C++17 when we enabled it, we never mentioned CTAD, so the only users of it are either the few programmers who actually know about it or accidental usages.
 
It is unfortunate that this warning will flag types designed to work with CTAD, but who don't need any explicit deduction guides.
Over time, I hope libraries that care about CTAD will address this by suppressing the warning manually; but that's not an ideal solution.

 
These classes, if I'm correct, are one of the selling point of CTAD and are prohibited thanks to this warning.

They're not meant to be triggered by this warning. It's just an oversight.

I'll update and audit libc++ to opt-in where needed. I'll also set up libc++'s test suite to build with this warning
enabled.

I'll try to get the fixed into 9.0.

I wasn't very explicit about this, however, we only use clang-cl with the MSVC headers. So from my point of view, it ain't that urgent. Although it technically being undefined behavior, we can write such a header as well and include it in our precompiled header (/FI).
Just wondering, is it allowed for the standard library implementation to add these deduction guides without them being specified in the standard? (I presume yes, as behavior doesn't change and one can't detect if this deduction guide exists)
 
 

To circle back to title of the email, should -Wctad-maybe-unsupported actually be a compiler warning iso a clang-tidy check?

(You mean "should -Wctad-maybe-unsupported be a clang-tidy check instead of a compiler warning?", I assume.)
Yes, indeed. My bad. 
 
If so, would it make sense of having a white-list, like all `std::` classes?

The diagnostic is no less valid for types in the standard library. The library author should audit the type to ensure it's CTAD safe, write tests, and then explicitly opt it in. This is
important for the correctness of client programs written using CTAD.


FWIW, I continue to believe that it makes no sense to talk about the library author "opting in" to CTAD.

Types have to be designed with CTAD in mine. Without explicit deduction guides CTAD often does the wrong thing.
Even otherwise unobservable qualities about how a constructor is lexically spelled in source can greatly impact CTAD.
Whether a template was designed to support CTAD is an important quality we should talk more about.

 
Either the client programmer trusts CTAD (even if it runs the risk of deducing the wrong thing sometimes) and thus every set-of-missing-angle-brackets must be assumed to be intentional no matter what type it's on, or else the client programmer doesn't trust CTAD (because of the aforementioned risk), and thus every set-of-missing-angle-brackets must be assumed to be unintentional.

This isn't a dichotomy.

To be clear, "-Wctad-maybe-unsupported" diagnoses only usages of CTAD on types without any explicit deduction guides.

  
The intentions of the library author don't matter at all in this equation.

 
Whether a type was designed with knowledge of CTAD in mind has a causal relationship with the correctness of usages of that type with CTAD.
As client programmer I don't want to use CTAD on types that haven't been changed since before CTAD was a feature.
Agreed! 
 


The situation for me is exactly analogous to VLAs in C — including the shape of the workaround: "write an explicit call to malloc()" versus "write an explicit call to make_foo()".
https://reviews.llvm.org/D54565 implements a `-Wctad` warning analogous to `-Wvla`.
 
I'm not sure we are talking about the same thing here, from what I read in that review, it flags all usages of CTAD. While the warning I mentioned doesn't trigger if the deduction guide exists. Although both use-cases are relevant, I'm in the luxurious position of having a single C++ standard to work with (and worry about).

–Arthur
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Challenging "-Wctad-maybe-unsupported" for 9.0.0

Kristóf Umann via cfe-dev
On Sun, Aug 4, 2019 at 5:25 AM JVApen <[hidden email]> wrote:
On Sun, 4 Aug 2019 at 00:37, Eric Fiselier <[hidden email]> wrote:
On Sat, Aug 3, 2019 at 10:14 AM Arthur O'Dwyer via cfe-dev <[hidden email]> wrote:
On Sat, Aug 3, 2019 at 6:03 AM JVApen via cfe-dev <[hidden email]> wrote:
Hello all,

I'm currently testing 9.0.0 and I'm one of those strange people that uses -Weverything in production, as we only support a single clang-release at the same time. (Actually clang-cl, as it's Windows-only)
During the upgrade, I've noticed a new warning "-Wctad-maybe-unsupported".
At first, I was really happy seeing it and started updating some code that implicitly used it.

Rewinding a bit: JVApen, could you explain a bit about why you felt the need to "update" code that used CTAD?
Also, you said "implicitly used" — is there any difference in your mind between "implicitly using" CTAD and "explicitly using" CTAD? What would the latter look like?

Over half of the updates I had to do were about `std::unique_lock`, `std::scope_guard` ... (and this without actively using this feature).

This implies that there were also some cases where people were using CTAD on types that weren't unique_lock or [I assume you meant scoped_lock/lock_guard, not scope_guard]. What's your opinion of those cases? While updating the code, did you find any usages of CTAD that were actually incorrect and/or unintentional?
How would you tell whether a use of CTAD was intentional or unintentional?


The situation for me is exactly analogous to VLAs in C — including the shape of the workaround: "write an explicit call to malloc()" versus "write an explicit call to make_foo()".
https://reviews.llvm.org/D54565 implements a `-Wctad` warning analogous to `-Wvla`.
 
I'm not sure we are talking about the same thing here, from what I read in that review, it flags all usages of CTAD. While the warning I mentioned doesn't trigger if the deduction guide exists. Although both use-cases are relevant, I'm in the luxurious position of having a single C++ standard to work with (and worry about). 

In my experience, the existence of a deduction guide (written by the library programmer, reflecting only the extent of the library programmer's imagination) has no bearing on whether any particular usage of CTAD (written by the client programmer, often indicating a typo or thinko) is correct or not.

–Arthur

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Challenging "-Wctad-maybe-unsupported" for 9.0.0

Kristóf Umann via cfe-dev
On Sun, 4 Aug 2019 at 17:02, Arthur O'Dwyer <[hidden email]> wrote:
On Sun, Aug 4, 2019 at 5:25 AM JVApen <[hidden email]> wrote:
On Sun, 4 Aug 2019 at 00:37, Eric Fiselier <[hidden email]> wrote:
On Sat, Aug 3, 2019 at 10:14 AM Arthur O'Dwyer via cfe-dev <[hidden email]> wrote:
On Sat, Aug 3, 2019 at 6:03 AM JVApen via cfe-dev <[hidden email]> wrote:
Hello all,

I'm currently testing 9.0.0 and I'm one of those strange people that uses -Weverything in production, as we only support a single clang-release at the same time. (Actually clang-cl, as it's Windows-only)
During the upgrade, I've noticed a new warning "-Wctad-maybe-unsupported".
At first, I was really happy seeing it and started updating some code that implicitly used it.

Rewinding a bit: JVApen, could you explain a bit about why you felt the need to "update" code that used CTAD?
Also, you said "implicitly used" — is there any difference in your mind between "implicitly using" CTAD and "explicitly using" CTAD? What would the latter look like?
At the moment, I don't have much reason to decide going either way. I'm trying to understand this new warning and if it's useful in our case.
As I'm testing 9.0.0-RC1, I just want the warnings-as-error to disappear to tackle the actual issues.
For the implicit part: As mentioned earlier, we didn't communicate about CTAD when introducing C++17, so right now I assume it's all implicitly introduced. For sure some are explicit by the few developers that follow the news about C++ standardization. I was planning to find out on Monday.
 

Over half of the updates I had to do were about `std::unique_lock`, `std::scope_guard` ... (and this without actively using this feature).

This implies that there were also some cases where people were using CTAD on types that weren't unique_lock or [I assume you meant scoped_lock/lock_guard, not scope_guard]. What's your opinion of those cases? While updating the code, did you find any usages of CTAD that were actually incorrect and/or unintentional?
How would you tell whether a use of CTAD was intentional or unintentional?

See previous: At the moment, I don't.
 

The situation for me is exactly analogous to VLAs in C — including the shape of the workaround: "write an explicit call to malloc()" versus "write an explicit call to make_foo()".
https://reviews.llvm.org/D54565 implements a `-Wctad` warning analogous to `-Wvla`.
 
I'm not sure we are talking about the same thing here, from what I read in that review, it flags all usages of CTAD. While the warning I mentioned doesn't trigger if the deduction guide exists. Although both use-cases are relevant, I'm in the luxurious position of having a single C++ standard to work with (and worry about). 

In my experience, the existence of a deduction guide (written by the library programmer, reflecting only the extent of the library programmer's imagination) has no bearing on whether any particular usage of CTAD (written by the client programmer, often indicating a typo or thinko) is correct or not.

–Arthur

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Challenging "-Wctad-maybe-unsupported" for 9.0.0

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev
As a general principle: we will not remove a disabled-by-default warning just because it's noisy in -Weverything builds. As a user of -Weverything, you are expected to turn off the warnings that you don't like. That's the deal.

For this warning in particular, we have multiple groups of users that have asked for it and see it as essential in order to have a maintainable large-scale codebase using CTAD. The warning prevents accidental dependence on the implementation details of other people's code, which puts this check in the same category as (for instance) access control; generally we want that kind of check to be performed in all builds and not done separately by a lint tool such as clang-tidy.

The false positives on standard library types (especially in standard library implementations we don't control) do seem like a problem, and maintaining an explicit list of such types on which the warning is suppressed seems like a reasonable solution to me.

On Sat, 3 Aug 2019 at 03:03, JVApen via cfe-dev <[hidden email]> wrote:
Hello all,

I'm currently testing 9.0.0 and I'm one of those strange people that uses -Weverything in production, as we only support a single clang-release at the same time. (Actually clang-cl, as it's Windows-only)
During the upgrade, I've noticed a new warning "-Wctad-maybe-unsupported".
At first, I was really happy seeing it and started updating some code that implicitly used it.
I'm even considering to propose an extension to our styleguide, as mentioned in the review of the warning: https://reviews.llvm.org/D56731
`Some style guides want to allow using CTAD only on types that "opt-in"`

However, after finishing my initial testing and applying some suppression and update locally, I'm changing my mind.
Over half of the updates I had to do were about `std::unique_lock`, `std::scope_guard` ... (and this without actively using this feature).
These classes, if I'm correct, are one of the selling point of CTAD and are prohibited thanks to this warning.

To circle back to title of the email, should -Wctad-maybe-unsupported actually be a compiler warning iso a clang-tidy check?
If so, would it make sense of having a white-list, like all `std::` classes?




_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Challenging "-Wctad-maybe-unsupported" for 9.0.0

Kristóf Umann via cfe-dev
Thanks all!

I totally agree with Richard about the functionality making sense. The more I read about CTAD, the more I see value in this warning.

When I wrote the first email, I wasn't even considering maintaining such a whitelist in code and wondered if it was at the right place as one could configure that whitelist in the config file.

I've now drafted a first version of a whitelist myself already, which doesn't look that terrible to maintain given our way of working. I think I've seen enough arguments to convince me that making this a clang-tidy check iso a warning ain't a good idea.

Thanks for all of your time!

On Sun, Aug 4, 2019, 22:55 Richard Smith <[hidden email]> wrote:
As a general principle: we will not remove a disabled-by-default warning just because it's noisy in -Weverything builds. As a user of -Weverything, you are expected to turn off the warnings that you don't like. That's the deal.

For this warning in particular, we have multiple groups of users that have asked for it and see it as essential in order to have a maintainable large-scale codebase using CTAD. The warning prevents accidental dependence on the implementation details of other people's code, which puts this check in the same category as (for instance) access control; generally we want that kind of check to be performed in all builds and not done separately by a lint tool such as clang-tidy.

The false positives on standard library types (especially in standard library implementations we don't control) do seem like a problem, and maintaining an explicit list of such types on which the warning is suppressed seems like a reasonable solution to me.

On Sat, 3 Aug 2019 at 03:03, JVApen via cfe-dev <[hidden email]> wrote:
Hello all,

I'm currently testing 9.0.0 and I'm one of those strange people that uses -Weverything in production, as we only support a single clang-release at the same time. (Actually clang-cl, as it's Windows-only)
During the upgrade, I've noticed a new warning "-Wctad-maybe-unsupported".
At first, I was really happy seeing it and started updating some code that implicitly used it.
I'm even considering to propose an extension to our styleguide, as mentioned in the review of the warning: https://reviews.llvm.org/D56731
`Some style guides want to allow using CTAD only on types that "opt-in"`

However, after finishing my initial testing and applying some suppression and update locally, I'm changing my mind.
Over half of the updates I had to do were about `std::unique_lock`, `std::scope_guard` ... (and this without actively using this feature).
These classes, if I'm correct, are one of the selling point of CTAD and are prohibited thanks to this warning.

To circle back to title of the email, should -Wctad-maybe-unsupported actually be a compiler warning iso a clang-tidy check?
If so, would it make sense of having a white-list, like all `std::` classes?




_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev