[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/Parse/Parser.h:2343
+  static ImplicitTypenameContext
+  isImplicitTypenameContext(DeclSpecContext DSC) {
+    switch (DSC) {
----------------
Starting with `is` implies contextual conversion to bool (generally), so I'd suggest renaming.


================
Comment at: clang/include/clang/Sema/DeclSpec.h:1806
+// typename is allowed (C++2a [temp.res]p5]).
+enum class ImplicitTypenameContext {
+  No,
----------------
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.)


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53847