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

[Alan Zhao via Phabricator via cfe-commits]

ayzhao marked 4 inline comments as done.
ayzhao added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:5592
+bool Parser::isConstructorDeclarator(bool IsUnqualified, bool DeductionGuide,
+                                     bool IsFriend) {
   TentativeParsingAction TPA(*this);
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > erichkeane wrote:
> > > > aaron.ballman wrote:
> > > > > shafik wrote:
> > > > > > 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.
> > > > > 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.
> > > > 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?
> > > 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.
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53847/new/

https://reviews.llvm.org/D53847