[PATCH] D54565: Introduce `-Wc++14-compat-ctad` as a subgroup of `-Wc++14-compat`

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

In https://reviews.llvm.org/D54565#1304740, @rsmith wrote:

> 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.


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.


Repository:
  rC Clang

https://reviews.llvm.org/D54565