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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
