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