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

[Abbas Sabra via Phabricator via cfe-commits]

AbbasSabra added a comment.

> Does MS use this in any library headers or such

I didn't check if they do. But if any codebase does use "abstract" and they are trying to use clang-cl they would face a parsing error.
My use case was running static analysis on code that uses this feature. Such an error had a great impact on the quality of the analysis.



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



================
Comment at: clang/lib/Sema/DeclSpec.cpp:1479
+  case VS_Abstract:
+    VS_abstractLoc = Loc;
+    break;
----------------
hans wrote:
> nit: the other cases just use one line
clang-format doesn't like it. Should I ignore it?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102517