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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Looking good overall, just a few nits.

Out of curiosity, where would one run into this? Does MS use this in any library headers or such?



================
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;
----------------
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.


================
Comment at: clang/lib/Sema/DeclSpec.cpp:1479
+  case VS_Abstract:
+    VS_abstractLoc = Loc;
+    break;
----------------
nit: the other cases just use one line


================
Comment at: clang/lib/Sema/DeclSpec.cpp:1493
   case VS_Sealed: return "sealed";
+  case VS_Abstract:
+    return "abstract";
----------------
nit: skip the newline for consistency here too


================
Comment at: clang/lib/Sema/SemaDecl.cpp:16449
 
-  if (FinalLoc.isValid())
+  if (IsAbstract) {
+    Record->markAbstract();
----------------
nit: it would be more common in clang to omit the braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102517