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