[PATCH] D102517: [clang] Add support for the "abstract" contextual keyword of MSVC

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Apologies for the slow reply, it was a long weekend here.



================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3314
   if (getLangOpts().CPlusPlus && Tok.is(tok::identifier)) {
-    VirtSpecifiers::Specifier Specifier = isCXX11VirtSpecifier(Tok);
-    assert((Specifier == VirtSpecifiers::VS_Final ||
-            Specifier == VirtSpecifiers::VS_GNU_Final ||
-            Specifier == VirtSpecifiers::VS_Sealed) &&
+    for (VirtSpecifiers::Specifier Specifier = isCXX11VirtSpecifier(Tok);
+         Specifier != VirtSpecifiers::VS_None;
----------------
AbbasSabra wrote:
> hans wrote:
> > This 'for' construct is hard to follow. I think it might be easier to follow as a while loop, maybe
> > 
> > ```
> > while ((Specifier = isCXX11VirtSpecifier(Tok)) != VirtSpecifiers::VS_None) {
> > ```
> > 
> > Also the "is" prefix in isCXX11VirtSpecifier is confusing given that it doesn't return a bool.
> For "for" vs "while" if you use while you will have to declare the variable before the loop giving it the wrong scope which makes it less readable in my opinion. 
> For  !!isCXX11VirtSpecifier!! I agree but I wouldn't separate the rename from this patch(I'm not sure what is the guidelines here)
> 
I still think the for-loop is confusing. How about something like this, then?

```
while (true) {
  VirtSpecifiers::Specifier Specifier = isCXX11VirtSpecifier(Tok);
  if (Specifier == VirtSpecifiers::VS_None)
    break;

  ...
}
```

I agree that renaming isCXX11VirtSpecifier in a separate patch is a good idea.


================
Comment at: clang/lib/Sema/DeclSpec.cpp:1479
+  case VS_Abstract:
+    VS_abstractLoc = Loc;
+    break;
----------------
AbbasSabra wrote:
> hans wrote:
> > nit: the other cases just use one line
> clang-format doesn't like it. Should I ignore it?
Yes, ignoring it is fine. (Re-formatting the whole switch would also be okay, as long as its consistent.)


================
Comment at: clang/lib/Sema/DeclSpec.cpp:1493
   case VS_Sealed: return "sealed";
+  case VS_Abstract:
+    return "abstract";
----------------
AbbasSabra wrote:
> hans wrote:
> > nit: skip the newline for consistency here too
> same clang-format splits the line
Ignoring clang-format is fine, sticking to the current style is usually preferred..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102517