Since I am interested in diagnosing this specific C++17ism (but not necessarily other C++17isms) in our codebase going forward.
The previous version of this patch named the option -Wc++14-compat-ctad, but I changed it to -Wctad per @rsmith's comments. Personally I see this as similar in spirit to -Wvla so the name -Wctad makes sense to me. My goal is just to have a fine-grained diagnostic by whatever name.
Details
Details
- Reviewers
rsmith jfb rjmccall aaron.ballman
Diff Detail
Diff Detail
- Repository
- rC Clang
Event Timeline
Comment Actions
Added a test case (partly copied from test/SemaCXX/cxx14-compat.cpp).
On the last warning in the file, I omitted the sketchy part of the warning, which on Godbolt looks like this:
https://godbolt.org/z/CB4z99
<source>:7:5: warning: class template argument deduction is incompatible with C++ standards before C++17; for compatibility, use explicit type name 'V<(lambda at <source>:6:16)>' [-Wc++98-c++11-c++14-compat]
V v(lam);
^
1 warning generated.This seems like a bug in the diagnostic, but fixing it is definitely not part of this patch, and probably above my pay grade at the moment anyway. :)
Comment Actions
In the past, we've been resistant to adding more fine-grained compat warnings, because we don't want to encourage subsetting the language (which sounds like exactly what you're trying to do here). We generally don't think it's Clang's business to enforce coding style conventions (such as "don't use CTAD because it makes your code less readable"), but we do consider it to be in-scope for Clang to warn on constructs that are error-prone or that have a negative impact on portability or compatibility, and so on. On that basis, I think there is a case to be made for warning on this specific language feature, because using CTAD on class templates that weren't designed for it is dangerous and creates source compatibility problems for future changes to that library.
However, if we consider the goal to be to warn only on using CTAD in places where the class template author didn't design for it, then:
- this should be a separate warning, not a subgroup of -Wc++14-compat
- the warning should be suppressed by any explicitly-declared deduction guides for the class
- there should be some fairly obvious way to suppress the warning in the case where the deduction guides implied by the constructors already do the right thing (maybe an attribute?)
What do you think? Would that cover the use case you're trying to address here, or are you really trying to enforce a style rule of "don't use CTAD ever"?
Comment Actions
Yes, that's a good way to put it.
Personally I would go stronger: using CTAD on class templates that were designed for it (1) is dangerous and (2) may create source compatibility problems for future changes to that library! The Standard Library itself deals with (2) by simply promising not to make future changes to the library... and deals with (1) by making changes to the library. For example, CTAD has in the "past/present" had problems distinguishing std::vector{"hello", "world"} (vector<const char*>, or UB?) from std::vector{set.begin(), set.end()} (vector<int>, or vector<set<int>::iterator>?). The guides for std::pair, and the guides around uses-allocator construction, seem particularly in flux, to my outside-observer eyes.
However, if we consider the goal to be to warn only on using CTAD in places where the class template author didn't design for it, then:
Unfortunately, I don't think the diagnostic will ever be able to detect "design"; the best it could do would be to detect "coding", which I don't think is good enough, in this case. ;)
- this should be a separate warning, not a subgroup of -Wc++14-compat
- the warning should be suppressed by any explicitly-declared deduction guides for the class
- there should be some fairly obvious way to suppress the warning in the case where the deduction guides implied by the constructors already do the right thing (maybe an attribute?)
What do you think? Would that cover the use case you're trying to address here, or are you really trying to enforce a style rule of "don't use CTAD ever"?
Essentially the latter.
- I would be quite happy to move the diagnostic outside of -Wc++14-compat.
- I specifically do not want the diagnostic to become suppressed on explicitly-declared deduction guides; that would have the effect of suppressing it for e.g. std::vector and std::pair. That would also have the effect of nerfing it for user-defined types: if Alice is relying on the diagnostic to avoid accidental CTAD, and then Bob adds a deduction guide that solves his case without perfectly solving Alice's case, then the diagnostic will go silent for Alice even though the consequences of Alice's slip-up are still just as severe.
- I think I don't understand your third bullet. My use-case requires a diagnostic about the call site, opted into by the writer of the call-site (Alice). IIUC, you ask for a way that the callee (the author of the class type, or perhaps Bob) could unilaterally suppress that diagnostic. But only the caller is able to make the judgment call that says "I don't trust these deduction guides (either they're incorrect, or I think they're prone to change in the next release, or both); I want a diagnostic every time I accidentally rely on one of them." The callee can't tell whether any particular use was accidental or intentional.
Comment Actions
(From a user point of view, I have no idea what "CTAD" means, and I sometimes work on a C++ compiler.)
Comment Actions
@rsmith ping!
@thakis: You said, "From a user point of view, I have no idea what "CTAD" means, and I sometimes work on a C++ compiler." Did you mean "I think the command-line option should be named something more verbose than -W[c++14-compat-]ctad"? Or "I don't know what CTAD is and I would indeed like a diagnostic if I use it accidentally"? Or something else?
Comment Actions
The former. Since I don't know what it means, I don't know if I want a diagnostic for it :-)
Comment Actions
Fair point. ;) But the full text of the diagnostic message does say "class template argument deduction is...," which does hint at the meaning of "CTAD."
I just checked and the text of the diagnostic for -Wvla is "variable length array used"; maybe I should follow its lead and just say "class template argument deduction used," thus even further divorcing this option from -Wc++14-compat.
Comment Actions
I think there is a case to be made for warning on this specific language feature, because using CTAD on class templates that weren't designed for it is dangerous and creates source compatibility problems for future changes to that library.
Personally I would go stronger: using CTAD on class templates that were designed for it (1) is dangerous and (2) may create source compatibility problems for future changes to that library!
I've given this a lot of thought, and while I'm very sympathetic to your desire here, I do not think that we have sufficient justification to provide a warning to allow (effectively) disabling this language features. There's an important difference here:
- using CTAD on arbitrary class templates that weren't designed for it creates a source compatibility problem that the class template author has no control over nor say in
- using CTAD on class templates that were designed for it does not create a new source compatibility problem -- the class template author created the source compatibility problem themselves when they designed how their class templates would interact with CTAD
Either way there's a source compatibility problem, but in the second case, it's the class template author's choice to extend their source compatibility contract. So:
- I still think it's a good idea to have a warning to warn on use of CTAD on a class where there is no evidence the class was designed for CTAD (this warning would be suppressed by either an explicit deduction guide or by an attribute on the class template definition indicating that the constructors are designed to support CTAD)
- it would also seem like a good idea to have warnings for CTAD cases that we have strong evidence are dangerous (eg, for T(x...) where x expands to a pack containing a single T, we should warn if we use the copy-deduction candidate and there's another candidate that would have been selected if the copy-deduction candidate didn't get a priority boost)
... but I don't think we can justify warning on all CTAD.
Have you considered the clang-tidy route? Given that it seems like your goal is to avoid accidental use of CTAD, an out-of-band (not part of regular compilations) check that you satisfy that goal would seem like it could be part of a solution.
Comment Actions
- using CTAD on arbitrary class templates that weren't designed for it creates a source compatibility problem that the class template author has no control over nor say in
- using CTAD on class templates that were designed for it does not create a new source compatibility problem -- the class template author created the source compatibility problem themselves when they designed how their class templates would interact with CTAD
I don't see the distinction you're making. It sounds like you're saying, "std::vector is a 'CTAD-enabled' type. You (the client programmer) opted into CTAD when you started using std::vector in your project. If you don't want accidental CTAD, stop using std::vector" — which I don't think is practically useful advice.
it would also seem like a good idea to have warnings for CTAD cases that we have strong evidence are dangerous (eg, for T(x...) where x expands to a pack containing a single T, we should warn if we use the copy-deduction candidate and there's another candidate that would have been selected if the copy-deduction candidate didn't get a priority boost)
That sounds like a good idea (although I am not qualified to implement it). I would also ask for a warning on T t{x}; within a generic algorithm where x is of some dependent type that might or might not happen to be a specialization of T. And ideally we'd be able to produce the warning at parse time (i.e. generic-algorithm-testing time) rather than waiting until the offending template instantiation is actually instantiated (i.e. generic-algorithm-usage-in-the-field time), because by then it's too late for the client programmer to do anything about it.
For an (artificial) example of how accidental CTAD in a generic algorithm can cause interesting misbehavior, see https://quuxplusone.github.io/blog/2018/12/11/dont-inherit-from-std-types/#now-we-feed-them-to-this-functio
Comment Actions
I doubt "CTAD" is going to survive as a term that people recognize and remember, and I don't think -Wclass-template-argument-deduction is outrageously wordy compared to some of our other diagnostic groups. With that change, this would be fine with me.
Comment Actions
Oh, I see this review might be dead in the water; I'm not sure why I was just added as a reviewer to it (by a non-author, no less).
I personally have no problem with pulling out specific features as sub-groups of the compatibility warning. I agree with Richard, though, that if the real goal is to warn about "unsafe" uses of this feature then a more tailored warning would be better.