Skip to content

[Clang] Add warning message for C++17 alias template CTAD #133806

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 3, 2025
Merged

[Clang] Add warning message for C++17 alias template CTAD #133806

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dd97898
Add warning message for C++17 alias template CTAD
GeorgeKA Mar 31, 2025
c48c4a6
Switched to using multiclasses for both messages
GeorgeKA Mar 31, 2025
01789a4
Adde release notes
GeorgeKA Apr 1, 2025
42a08fb
Merge branch 'main' into 125913-warning_msg_for_cxx17_alias_template_…
GeorgeKA Apr 1, 2025
5299a58
Merge branch 'llvm:main' into 125913-warning_msg_for_cxx17_alias_temp…
GeorgeKA Apr 2, 2025
df703ea
Switched to using DiagCompat for compatibility messages
GeorgeKA Apr 2, 2025
eec39e4
Merge branch 'main' into 125913-warning_msg_for_cxx17_alias_template_…
GeorgeKA Apr 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ defm constexpr_ctor_missing_init : CXX20Compat<
defm adl_only_template_id : CXX20Compat<
"use of function template name with no prior declaration in function call "
"with explicit template arguments is">;
defm ctad_for_alias_templates
Copy link
Collaborator

@shafik shafik Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sirraide while I get the "efficiency" here I am bit uncomfortable that as a reviewer it is no longer obvious what diagnostics I need to look for in tests and as we have seen here easy to overlook that we are not fully covering tests.

This feels very anti-ergonomic for code reviewers. Maybe worth discussing during wg meeting CC @AaronBallman

Copy link
Member

@Sirraide Sirraide Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels very anti-ergonomic for code reviewers.

I feel like that might mainly be because this is a new thing we just added. I personally don’t think it’s that big of a deal because the wording of compatibility warnings is very consistent as a result of this, so so long as it’s clear that this always generates something of the form ‘... incompatibile with C++ standards before C++XY’ and ‘... a C++XY extension’, you know what to look for.

Also, even if it adds another thing to be aware of, it also removes the need to check if the compatibility warning is actually written correctly and whether we’re checking for the right lang opt when we emit it etc. etc. (though of course that last part will only come into effect once #132348 is merged).

: CXX20Compat<"class template argument deduction for alias templates is">;

// C++23 compatibility with C++20 and earlier.
defm constexpr_static_var : CXX23Compat<
Expand Down Expand Up @@ -8442,11 +8444,6 @@ let CategoryName = "Lambda Issue" in {
def warn_cxx17_compat_lambda_def_ctor_assign : Warning<
"%select{default construction|assignment}0 of lambda is incompatible with "
"C++ standards before C++20">, InGroup<CXXPre20Compat>, DefaultIgnore;

// C++20 class template argument deduction for alias templates.
def warn_cxx17_compat_ctad_for_alias_templates : Warning<
"class template argument deduction for alias templates is incompatible with "
"C++ standards before C++20">, InGroup<CXXPre20Compat>, DefaultIgnore;
}

def err_return_in_captured_stmt : Error<
Expand Down
4 changes: 3 additions & 1 deletion clang/lib/Sema/SemaInit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9896,7 +9896,9 @@ QualType Sema::DeduceTemplateSpecializationFromInitializer(
if (auto *AliasTemplate = dyn_cast_or_null<TypeAliasTemplateDecl>(
TemplateName.getAsTemplateDecl())) {
Diag(Kind.getLocation(),
diag::warn_cxx17_compat_ctad_for_alias_templates);
getLangOpts().CPlusPlus20
Copy link
Collaborator

@erichkeane erichkeane Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sirraide : Didn't we have a function you were making to get the right diagnostic here? Should/could we use that here?

Copy link
Member

@Sirraide Sirraide Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can use DiagCompat() here once #132348 is merged (which should happen in a few hours barring unforeseen circumstances). I’ll leave another comment once that has happened.

Copy link
Collaborator

@erichkeane erichkeane Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'd vastly prefer we hold off on this patch until that is merged and rebase it with that. I hadn't realized that 132348 hadn't been merged yet.

Copy link
Member

@Sirraide Sirraide Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been merged since (I had to revert it once but I’ve already relanded it)

Copy link
Collaborator

@erichkeane erichkeane Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah :) I was just blaming my bad memory. I just got back from a week off, so my brain isn't particularly caught up yet :D

Copy link
Member

@Sirraide Sirraide Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah :) I was just blaming my bad memory. I just got back from a week off, so my brain isn't particularly caught up yet :D

I merged it a few hours ago so you’re good ;Þ

? diag::compat_cxx20_ctad_for_alias_templates
: diag::compat_pre_cxx20_ctad_for_alias_templates);
Copy link
Collaborator

@shafik shafik Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we check both of these branches in the test below? It does not look like it, we need to make sure we completely cover all new code in tests.

Copy link
Contributor Author

@GeorgeKA GeorgeKA Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the checks for each condition. Is there something additional you're looking for?

diag::compat_pre_cxx20_ctad_for_alias_templates:

llvm-project/clang/test/SemaCXX/cxx17-compat.cpp

Line 140 in 42a08fb

// expected-warning@-2 {{class template argument deduction for alias templates is a C++20 extension}}

diag::compat_cxx20_ctad_for_alias_templates:

llvm-project/clang/test/SemaCXX/cxx17-compat.cpp

Line 142 in 42a08fb

// expected-warning@-4 {{class template argument deduction for alias templates is incompatible with C++ standards before C++20}}

LookupTemplateDecl = AliasTemplate;
auto UnderlyingType = AliasTemplate->getTemplatedDecl()
->getUnderlyingType()
Expand Down
2 changes: 1 addition & 1 deletion clang/test/SemaCXX/cxx17-compat.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ template<typename T> struct A { A(T); };
template<typename T> using B = A<T>;
B b = {1};
#if __cplusplus <= 201703L
// FIXME: diagnose as well
// expected-warning@-2 {{class template argument deduction for alias templates is a C++20 extension}}
#else
// expected-warning@-4 {{class template argument deduction for alias templates is incompatible with C++ standards before C++20}}
#endif
Expand Down
4 changes: 2 additions & 2 deletions clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,9 @@ namespace dependent {
};
template<typename T> void f() {
typename T::X tx = 0;
typename T::Y ty = 0;
typename T::Y ty = 0; // expected-warning {{class template argument deduction for alias templates is a C++20 extension}}
}
template void f<B>();
template void f<B>(); // expected-note {{in instantiation of function template specialization 'dependent::f<dependent::B>' requested here}}

template<typename T> struct C { C(T); };
template<typename T> C(T) -> C<T>;
Expand Down