This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Suppress -Wctad-maybe-unsupported on types w/o deduction guides
ClosedPublic

Authored by ldionne on Sep 8 2022, 3:06 PM.

Details

Summary

There are a handful of standard library types that are intended
to support CTAD but don't need any explicit deduction guides to
do so.

This patch adds a dummy deduction guide to those types to suppress
-Wctad-maybe-unsupported (which gets emitted in user code).

This is a re-application of the original patch by Eric Fiselier in
fcd549a7d828 which had been reverted due to reasons lost at this point.
I also added the macro to a few more types. Reviving this patch was
prompted by the discussion on https://llvm.org/D133425.

Diff Detail

Event Timeline

ldionne created this revision.Sep 8 2022, 3:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2022, 3:06 PM
Herald added a subscriber: arichardson. · View Herald Transcript
ldionne requested review of this revision.Sep 8 2022, 3:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2022, 3:06 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
EricWF accepted this revision.Sep 9 2022, 8:14 AM
This revision is now accepted and ready to land.Sep 9 2022, 8:14 AM

I noticed the original patch was from 2019 and thus doesn't adds the markers for the types in std::format. Using a recent Clang seems to fails to build our benchmarks. The errors look the same as the C++2b CI failure.

Otherwise the patch LGTM!

ldionne updated this revision to Diff 462277.Sep 22 2022, 1:14 PM

Update several types that were missing the annotation.

ldionne updated this revision to Diff 462918.Sep 26 2022, 8:28 AM

Rebase, trigger CI and remove warning suppression from libc++'s own build.

ldionne updated this revision to Diff 463884.Sep 29 2022, 7:03 AM

Fix C++11/C++03

ldionne updated this revision to Diff 463993.Sep 29 2022, 12:13 PM

Address issue in C++17

ldionne updated this revision to Diff 464058.Sep 29 2022, 3:06 PM

Remove variant test. @Mordante you uncommented that and it failed with this patch due to CTAD.

I was going to add an allowance for variant to silence the warning, and then I thought to myself: does it really make sense?
I can't think of any other CTAD with variant that would make sense other than the copy constructor, which seems extremely weak,
since CTAD always makes sense with a copy ctor.

In particular, I think the code std::variant v{3} is extremely suspicious (why on earth would you create a variant
that always contains an int -- this must usually be a mistake). So instead, I'm tempted to let the warning warn when
CTAD is used with variant.

In particular, I think the code std::variant v{3} is extremely suspicious (why on earth would you create a variant
that always contains an int -- this must usually be a mistake). So instead, I'm tempted to let the warning warn when
CTAD is used with variant.

I think there are valid cases.

#ifdef FOO
std::variant<int, double> v{foo(...);};
#else
std::variant<int, double> v{42};
#endif

But since it's only a warning users can just compile without the flag and be happy. So I don't think we need to support this feature. But I feel the test should be fixed instead of removed.

I like the format contexts also to keep using CTAD, I don't feel writing the template types there makes the code easier to understand.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp
47

nice catch.

libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/ctor.pass.cpp
43

Here too I prefer CTAD to work.

libcxx/test/std/utilities/variant/variant.variant/variant.ctor/copy.pass.cpp
258 ↗(On Diff #464058)

This line is what is tested, this is no longer ill-formed. So I would prefer to keep this test and instead adjust the construction.

I wonder how many libc++ users use this construct. Does it only warn when the types are convertible or always?
For example will this also generate a diagnostic?

std::variant<int, std::string_view> v1(3);
ldionne marked 2 inline comments as done.Sep 30 2022, 2:28 PM

I like the format contexts also to keep using CTAD, I don't feel writing the template types there makes the code easier to understand.

Instead, I would suggest going through all the types in format and considering which ones it makes sense to use CTAD with. Does that sound reasonable? This patch is only trying to get the library+tests compiling with the warning enabled with as little changes as possible.

ldionne updated this revision to Diff 464404.Sep 30 2022, 2:29 PM

Fix backdeployment issue.

I like the format contexts also to keep using CTAD, I don't feel writing the template types there makes the code easier to understand.

Instead, I would suggest going through all the types in format and considering which ones it makes sense to use CTAD with. Does that sound reasonable? This patch is only trying to get the library+tests compiling with the warning enabled with as little changes as possible.

These types are public types, so we can't just add CTAD to them. But I wouldn't mind to commit the format part as-is and afterwards I rework the format patch in a followup. (This followup will be a partial revert of this commit.)

I like the format contexts also to keep using CTAD, I don't feel writing the template types there makes the code easier to understand.

Instead, I would suggest going through all the types in format and considering which ones it makes sense to use CTAD with. Does that sound reasonable? This patch is only trying to get the library+tests compiling with the warning enabled with as little changes as possible.

These types are public types, so we can't just add CTAD to them. But I wouldn't mind to commit the format part as-is and afterwards I rework the format patch in a followup. (This followup will be a partial revert of this commit.)

Yes, I think that's fine. We wouldn't add CTAD to those types, we would simply add the _LIBCPP_CTAD_ENABLED_FOR_TYPE(foo); fake deduction guide to them. That should be conforming.

This wasn't actually reverted, @Mordante 's change only added more types that support CTAD.