Skip to content
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

[clang][Sema] Fix a CTAD regression after 42239d2e9 #86914

Merged
merged 4 commits into from
Mar 29, 2024
Merged

[clang][Sema] Fix a CTAD regression after 42239d2e9 #86914

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b31ca78
[clang][Sema] Fix a CTAD regression after 42239d2e9
zyn0217 Mar 28, 2024
6acd294
Expand comments
zyn0217 Mar 29, 2024
3403c76
Mention N3337 in the test.
zyn0217 Mar 29, 2024
6607f7b
Spell out C++11
zyn0217 Mar 29, 2024
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
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,9 @@ Bug Fixes in This Version
- Fixes an assertion failure on invalid code when trying to define member
functions in lambdas.

- Fixed a regression in CTAD that a friend declaration that befriends itself may cause
incorrect constraint substitution. (#GH86769).

Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Expand Down
14 changes: 13 additions & 1 deletion clang/lib/Sema/SemaTemplate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1836,7 +1836,19 @@ static TemplateParameterList *GetTemplateParameterList(TemplateDecl *TD) {
// Make sure we get the template parameter list from the most
// recent declaration, since that is the only one that is guaranteed to
// have all the default template argument information.
return cast<TemplateDecl>(TD->getMostRecentDecl())->getTemplateParameters();
Decl *ND = TD->getMostRecentDecl();
// Skip past friend Decls because they are not supposed to contain default
// template arguments. Moreover, these declarations may introduce template
// parameters living in different template depths than the corresponding
// template parameters in TD, causing unmatched constraint substitution.
//
// C++23 N4950 [temp.param]p12
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this is not a new thing in C++23, I also find it in the C++20 spec N4860. I'd remove the C++23 N4950.

And I think this is also a FIXME, clang should diagnose this error case, maybe add the FIXME in the comment.

I think this function is not only used for class templates, but also for function templates. For function templates, there is an exception per the standard (temp.param 12): If a friend function template declaration specifies a default template-argument, that declaration shall be a definition and shall be the only declaration of the function template in the translation unit.

Copy link
Member

Choose a reason for hiding this comment

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

I think this function is not only used for class templates, but also for function templates. For function templates, there is an exception per the standard (temp.param 12): If a friend function template declaration specifies a default template-argument, that declaration shall be a definition and shall be the only declaration of the function template in the translation unit.

In that case it’d make sense to add a test for a friend function template as well if there isn’t already one to make sure this change doesn’t break that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: this is not a new thing in C++23, I also find it in the C++20 spec N4860. I'd remove the C++23 N4950.

I referenced that number because I'm concerned the paragraphs/wordings could vary across standards. :)
(Maybe I should use an earlier standard file number where this line first appears...?)

Copy link
Member

Choose a reason for hiding this comment

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

I referenced that number because I'm concerned the paragraphs/wordings could vary across standards. :) (Maybe I should use an earlier standard file number where this line first appears...?)

So apparently, the paragraphs do change sometimes, so if you want to cite a specific paragraph, then yeah, I’d include the standard version, but maybe cite the C++20 then in this case if it was added in C++20.

Copy link
Contributor Author

@zyn0217 zyn0217 Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In that case it’d make sense to add a test for a friend function template as well if there isn’t already one to make sure this change doesn’t break that.

I think the change is probably fine for friend function templates with default template parameters: they're unique across one TU, and we have ensured the next declaration we visit is non-null. So in such cases, I think ND would be equivalent to "the most recent Decl".

but maybe cite the C++20 then in this case if it was added in C++20.

Finally, I decided to use N3337 (aka C++11) since I couldn't find any related words prior to that draft, although gcc and clang don't allow such default arguments even in 98/03 mode.

Copy link
Member

Choose a reason for hiding this comment

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

At least I can’t find a mention of ‘N3337’ anywhere else in the codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn’t ‘N3337’ just the last draft of C++11? Just ‘C++11’ should be enough then?

https://github.com/timsong-cpp/cppwp says N3337 is the one with editorial fixes. So, I think it is.

Copy link
Member

Choose a reason for hiding this comment

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

I’d usually not include e.g. ‘N3337’ then unless it’s something that was changed specifically in that version of the standard as opposed to a previous version. If it’s the most up-to-date version, then just e.g. ‘C++11’ in this case would have been enough imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least I can’t find a mention of ‘N3337’ anywhere else in the codebase

Interestingly, I found people tend not to spell out the standard file number throughout the codebase - seemingly, we usually speak c++(version) [paragraph]/X.Y.

(I will respect such a convention next time; thanks for reminding me!)

Copy link
Member

Choose a reason for hiding this comment

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

I found people tend not to spell out the standard file number throughout the codebase

Probably beacuse most people don’t have all of them memorised (at least I don’t), whereas e.g. ‘C++11’ makes it pretty clear what we’re actually talking about.

// A default template argument shall not be specified in a friend class
// template declaration.
while (ND->getFriendObjectKind() != Decl::FriendObjectKind::FOK_None &&
ND->getPreviousDecl())
ND = ND->getPreviousDecl();
return cast<TemplateDecl>(ND)->getTemplateParameters();
}

DeclResult Sema::CheckClassTemplate(
Expand Down
24 changes: 24 additions & 0 deletions clang/test/SemaTemplate/concepts-friends.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,3 +478,27 @@ template <Concept> class Foo {
};

} // namespace FriendOfFriend

namespace GH86769 {

template <typename T>
concept X = true;

template <X T> struct Y {
Y(T) {}
template <X U> friend struct Y;
template <X U> friend struct Y;
template <X U> friend struct Y;
};

template <class T>
struct Z {
// FIXME: This is ill-formed per N4950 [temp.param]p12.
template <X U = void> friend struct Y;
};

template struct Y<int>;
template struct Z<int>;
Y y(1);

}
2 changes: 1 addition & 1 deletion clang/test/SemaTemplate/ctad.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,4 @@ X x;
template<class T, class B> struct Y { Y(T); };
template<class T, class B=void> struct Y ;
Y y(1);
};
}
Loading