https://docs.microsoft.com/en-us/cpp/extensions/abstract-cpp-component-extensions?view=msvc-160
Note: like the already supported "sealed" keyword, the "abstract" keyword is supported by MSVC by default.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?
clang/lib/Parse/ParseDeclCXX.cpp | ||
---|---|---|
3314–3352 | 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. | |
clang/lib/Sema/DeclSpec.cpp | ||
1479 | nit: the other cases just use one line | |
1491 | nit: skip the newline for consistency here too | |
clang/lib/Sema/SemaDecl.cpp | ||
16449–16452 | nit: it would be more common in clang to omit the braces |
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.
clang/lib/Parse/ParseDeclCXX.cpp | ||
---|---|---|
3314–3352 | 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. | |
clang/lib/Sema/DeclSpec.cpp | ||
1479 | clang-format doesn't like it. Should I ignore it? | |
1491 | same clang-format splits the line |
Apologies for the slow reply, it was a long weekend here.
clang/lib/Parse/ParseDeclCXX.cpp | ||
---|---|---|
3314–3352 | 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. | |
clang/lib/Sema/DeclSpec.cpp | ||
1479 | Yes, ignoring it is fine. (Re-formatting the whole switch would also be okay, as long as its consistent.) | |
1491 | Ignoring clang-format is fine, sticking to the current style is usually preferred.. |
Sorry, had to revert it as this fails under sanitizer : https://lab.llvm.org/buildbot/#/builders/5/builds/8150
Thanks!
I missed adding "Ident_abstract" in the Parser initialize function. Should be fixed now. CC: @hans
Thanks for the quick fix! Re-landed as 116179c2ee5213f2ae8f07a400ac98f0c995b3d3. I'll try to keep an eye on that buildbot.
This 'for' construct is hard to follow. I think it might be easier to follow as a while loop, maybe
Also the "is" prefix in isCXX11VirtSpecifier is confusing given that it doesn't return a bool.