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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Sema/DeclSpec.h:1806
+// typename is allowed (C++2a [temp.res]p5]).
+enum class ImplicitTypenameContext {
+  No,
----------------
ayzhao wrote:
> aaron.ballman wrote:
> > 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.)
> Ack. IIRC this `enum` was created as a result of [this comment](https://reviews.llvm.org/D53847?id=198139#inline-545979) by @rsmith expressing concern over adding an additional `bool` parameter to a function with a lot of preexisting `bool` args.
> 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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53847