Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[C++2a] P0634r3: Down with typename!
ClosedPublic

Authored by ayzhao on Oct 29 2018, 4:16 PM.

Details

Summary

This patch implements P0634r3 that removes the need for 'typename' in certain contexts.

For example,

template <typename T>
using foo = T::type; // ok

This is also allowed in previous language versions as an extension, because I think it's pretty useful. :)

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

hacky fix for the test CXX/module/module.interface/p2-2.cpp

If anyone has a better idea on how to fix this test, feedback is very welcome.

ayzhao added inline comments.Sep 16 2022, 4:40 PM
clang/lib/Sema/SemaTemplate.cpp
4089–4091

Currently looking into this.

It is interesting to note that for the following example in one of the parent comments:

template<typename T> struct X {
  template<typename U> struct Y { Y(); typedef void V; }; 
  template<typename U> typename Y<U>::V::W f();
};
template<typename T> template<typename U>
X<T>::template Y<U>::V::W X<T>::f() { return 0; }

GCC doesn't like the fact that the template keyword is on the final line if compiled with -pedantic [0]:

<source>:6:16: warning: keyword 'template' not allowed in declarator-id [-Wpedantic]
    6 | X<T>::template Y<U>::V::W X<T>::f() { return 0; }
      |                ^~~~
Compiler returned: 0

[0]: https://godbolt.org/z/dYddW3ErY

ayzhao updated this revision to Diff 461401.Sep 19 2022, 4:08 PM

fix typo

ayzhao updated this revision to Diff 461403.Sep 19 2022, 4:15 PM

remove extra blank line

cjdb removed a subscriber: cjdb.Sep 19 2022, 5:04 PM
ayzhao marked an inline comment as done.Sep 20 2022, 11:15 AM
ayzhao added inline comments.
clang/lib/Sema/SemaTemplate.cpp
4089–4091

This is starting to feel like a separate issue. I filed https://github.com/llvm/llvm-project/issues/57853 to track it.

ayzhao updated this revision to Diff 461699.Sep 20 2022, 2:02 PM

rebase + clang-format

ayzhao updated this revision to Diff 461729.Sep 20 2022, 3:23 PM
ayzhao marked 2 inline comments as done.

remove PrevLookupResult + put Declarator(...) and ~Declarator() definitions back in the header files to reduce the diff since we're no longer overloading the constructor.

ayzhao added inline comments.Sep 20 2022, 3:24 PM
clang/include/clang/Sema/DeclSpec.h
1924

Removed

clang/lib/Sema/DeclSpec.cpp
1502–1504 ↗(On Diff #458580)

Ended up removing PrevLookupResult per my other comment.

ayzhao updated this revision to Diff 462032.Sep 21 2022, 3:46 PM

rebase + upload; also friendly ping!

I made mostly small comments but I think @aaron.ballman and/or @erichkeane should take a look as well.

clang/lib/Parse/ParseDecl.cpp
3434–3437
5593

Instead of adding yet another bool flag maybe we can consider using something like enum isFriend : bool {No, Yes}.

I am sure @aaron.ballman will want to chime in here as well.

clang/lib/Parse/ParseTemplate.cpp
20

Do we still need this?

1449
clang/lib/Sema/Sema.cpp
51

Do we need this?

I think this is on the right track, but would like to see more work done on that fixme before we commit (or at least better understand what is causing this). I'm afraid of what else can get us into that situation besides the export-decl.

clang/lib/Sema/SemaDecl.cpp
367

Hmm... this scares me quite a bit, I don't really know what other fallout results from this, or other code that would hit here.

We should probably spend more time making sure we understand this better.

ayzhao updated this revision to Diff 462222.Sep 22 2022, 10:08 AM
ayzhao marked 2 inline comments as done.

address some review comments

aaron.ballman added inline comments.Sep 22 2022, 10:33 AM
clang/include/clang/Parse/Parser.h
2343

Starting with is implies contextual conversion to bool (generally), so I'd suggest renaming.

clang/include/clang/Sema/DeclSpec.h
1814

Not opposed to this construct, but I am worried about how well it will scale. I don't know that we want to add a bunch of named enums that all boil down to a bool. (If someone thinks they have good ideas here, that'd be a good RFC topic for Discourse because we have a ton of interfaces that take a bunch of bools.)

clang/lib/Parse/ParseDecl.cpp
5593

Heh, so this is where I get worried about the scalability of using enums for these. We really want to use three different enums here, but do we really want to *add* three different enums? I'm unconvinced.

However, if we can come up with some template magic to allow for named bool parameters as a generic interface, that would be valuable to use.

erichkeane added inline comments.Sep 22 2022, 10:44 AM
clang/lib/Parse/ParseDecl.cpp
5593

I prefer enums over bools TBH, even if we end up with a million of then somewhere.

That said, what about:

https://godbolt.org/z/Kz6jdjobj

template<typename SpecificThing>
class Is {
    Is(bool v) : value(v){}
  public:
  bool value;
  static const Is Yes() { return Is{true};}
  static const Is No() { return Is{false};}

  operator bool() { return value; }
};

class Friend{}; // #1
 
void foo(Is<Friend> f) {
    if (f) {
        ///...
    }
}

void baz() {
    foo(Is<Friend>::Yes());
}

Adding a 'new' thing is as simple as just adding #1 for anything we care about. We might want to put them in a namespace of some sort, but perhaps not awful?

aaron.ballman added inline comments.Sep 22 2022, 10:49 AM
clang/lib/Parse/ParseDecl.cpp
5593

Yeah, this is along the lines of what I was thinking of! However, I'm still concerned about that approach because it involves adding a new type for every situation we have a bool. Empty classes to use as a tag definitely works, but I was hoping we could use a string literal rather than a tag type so that we don't have the extra compile time overhead of adding hundreds of new empty classes. e.g.,

void foo(Is<"Friend"> f) {
  if (f) {
     // ...
  }
}

void baz() {
  foo(Is<"Friend">::Yes); // Yay
  foo(Is<"Enemy">::Yes); // Error for type mismatch with Is<"Friend">
}

However, that might require compiling with C++20 (I don't recall), so it may not be a viable idea.

ayzhao updated this revision to Diff 462239.Sep 22 2022, 10:51 AM
ayzhao marked 2 inline comments as done.

address some comments

ayzhao added inline comments.Sep 22 2022, 10:53 AM
clang/include/clang/Sema/DeclSpec.h
1814

Ack. IIRC this enum was created as a result of this comment by @rsmith expressing concern over adding an additional bool parameter to a function with a lot of preexisting bool args.

erichkeane added inline comments.Sep 22 2022, 10:54 AM
clang/lib/Parse/ParseDecl.cpp
5593

Yeah, referring to stringliterals is troublesome in C++17. However, we COULD do that like:

void foo(Is<"Friend"_Is> f) {
  if (f) {
     // ...
  }
}

void baz() {
  foo(Is<"Friend"_Is>::Yes); // Yay
  foo(Is<"Enemy"_Is>::Yes); // Error for type mismatch with Is<"Friend">
}

by making operator _Is return an integer_sequence.

aaron.ballman added inline comments.Sep 22 2022, 11:02 AM
clang/include/clang/Sema/DeclSpec.h
1814

Ack. IIRC this enum was created as a result of this comment by @rsmith expressing concern over adding an additional bool parameter to a function with a lot of preexisting bool args.

Yeah, I spotted his suggestion (and am totally fine with implementing it here as you've done). I was mostly thinking about the future when someone tries to do this for every set of 2 or more bool parameters. :-D

clang/lib/Parse/ParseDecl.cpp
5593

That's a really neat idea! If you want to work it up into something that could be plausible to add to ADT, I think it's worth an RFC to add the interface. I'm guessing the diagnostic behavior of that would be kind of gross, but once we move to C++20 we'd be able to use string literal template arguments directly and get better diagnostic behavior. The critical part is that the code is readable and we get diagnostics when passing an argument to the wrong parameter.

ayzhao updated this revision to Diff 462315.Sep 22 2022, 3:17 PM

Add FriendSpecified enum

ayzhao updated this revision to Diff 462317.Sep 22 2022, 3:18 PM

fix spacing

ayzhao marked 4 inline comments as done.Sep 22 2022, 3:25 PM
ayzhao added inline comments.
clang/lib/Parse/ParseDecl.cpp
5593

I added a FriendSpecified enum for now.

I'm going to mark the comment chain as done so that they don't unnecessarily block the review.

I agree with the idea of a generic type to represent boolean arguments, but I think it would be out of scope of this patch and that Discourse would be a more appropriate place to discuss this.

ayzhao marked 2 inline comments as done.Sep 23 2022, 2:11 PM
ayzhao updated this revision to Diff 462587.Sep 23 2022, 2:15 PM
ayzhao marked an inline comment as done.

address some more review comments + rebase

ayzhao added inline comments.Sep 23 2022, 4:29 PM
clang/lib/Sema/SemaDecl.cpp
367

Following up with my previous comment about the failing test that this hack was supposed to fix:

I added the typename keyword to the export template statement and lo and behold, Clang emits the same "declaration does not declare anything" diagnostic instead of the namespace scope diagnostic [0].

To me, this feels like that the original test case was wrong:

  1. There should've been a typename keyword in the export statement. Perhaps the author of the test wasn't aware that the typename keyword was required, or perhaps the author already assumed that P0634r3 was already implemented in clang.
  2. The test currently passing (with Clang emitting the namespace diagnostic instead of the declaration diagnostic) seems to be a freak coincidence.

I'm not familiar at all with C++ modules - is this a bug with exporting types?

[0]: https://godbolt.org/z/PYvh68Tq7

ayzhao marked 2 inline comments as done.Sep 23 2022, 4:39 PM
ayzhao added inline comments.
clang/lib/Sema/SemaDecl.cpp
367

small fix: I linked to the wrong line when referencing the original test case. The link should be https://github.com/llvm/llvm-project/blob/a707675dbba9ca3ec6e668f86fea2240a85ca171/clang/test/CXX/module/module.interface/p2-2.cpp#L17

ayzhao updated this revision to Diff 462633.Sep 23 2022, 5:47 PM

remove the hack for the test p2-2.cpp and patch in patch in D134578 which fixes the test

ayzhao marked an inline comment as done.Sep 23 2022, 5:50 PM
ayzhao added inline comments.
clang/lib/Sema/SemaDecl.cpp
367

So my belief now is that the test p2-2.cpp is broken. I've removed the FIXME hack, and I created D134578 to fix the test. To keep the bots happy, I also cherry-picked D134578 into this patch.

I think this is on the right track, but would like to see more work done on that fixme before we commit (or at least better understand what is causing this). I'm afraid of what else can get us into that situation besides the export-decl.

The FIXME has been removed; the problem seems to be that the test is broken. See https://reviews.llvm.org/D53847?id=462032#inline-1297092

erichkeane accepted this revision.Sep 26 2022, 6:21 AM

Thanks for looking into it. I think you're right that it is a mistake that we accepted that without the 'struct' keyword after a quick look, so I think I am OK with this. Please give @aaron.ballman a day or so to take a look if he wishes.

This revision is now accepted and ready to land.Sep 26 2022, 6:21 AM
ayzhao updated this revision to Diff 462969.Sep 26 2022, 11:01 AM

clang-format + rebase

It's great to see this make progress.
Can you update cxx_status.html and ReleaseNotes.txt?

ayzhao updated this revision to Diff 463334.Sep 27 2022, 1:59 PM

update cxx_status.html and ReleaseNotes.rst + rebase

It's great to see this make progress.
Can you update cxx_status.html and ReleaseNotes.txt?

Done.

I also saw that P2092R0 was marked as partially supported in cxx_status.html due to missing support for P0634R3, but I'm not familiar enough with P2092R0 to update that entry.

Good catch

template<typename T>
concept K = requires (typename T::Type X) { // (3)
X.next();
}

That typename should be optional. Can you add a test for that? If it works you can mark P2092 as fully supported.

Good catch

template<typename T>
concept K = requires (typename T::Type X) { // (3)
X.next();
}

That typename should be optional. Can you add a test for that? If it works you can mark P2092 as fully supported.

Done.

The test originally failed, but it was a simple fix to get it to work.

ayzhao updated this revision to Diff 463351.Sep 27 2022, 3:42 PM

add test and fix for P2092

add test and fix for P2092

These changes look good to me, thanks

erichkeane accepted this revision.Sep 28 2022, 6:12 AM
This revision was landed with ongoing or failed builds.Sep 28 2022, 9:50 AM
This revision was automatically updated to reflect the committed changes.