Page MenuHomePhabricator

[clang] Add support for the "abstract" contextual keyword of MSVC
ClosedPublic

Authored by AbbasSabra on May 14 2021, 10:58 AM.

Diff Detail

Event Timeline

AbbasSabra requested review of this revision.May 14 2021, 10:58 AM
AbbasSabra created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2021, 10:58 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
AbbasSabra edited the summary of this revision. (Show Details)May 17 2021, 1:00 AM
AbbasSabra added reviewers: majnemer, rnk, rsmith.
rnk edited reviewers, added: hans; removed: majnemer, rnk.May 20 2021, 10:53 AM
rnk added a subscriber: rnk.

@hans, can you review this? I am trying to offload clang reviews.

hans added a comment.May 21 2021, 1:31 AM

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–3353

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
16469–16472

nit: it would be more common in clang to omit the braces

AbbasSabra marked 3 inline comments as done.

Updating D102517: [clang]Apply code review

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–3353

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)

clang/lib/Sema/DeclSpec.cpp
1479

clang-format doesn't like it. Should I ignore it?

1491

same clang-format splits the line

hans added a comment.Tue, May 25, 1:19 AM

Apologies for the slow reply, it was a long weekend here.

clang/lib/Parse/ParseDeclCXX.cpp
3314–3353

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..

Updating D102517: [clang] Apply code review: while loop instead of for loop

AbbasSabra marked 4 inline comments as done.Wed, May 26, 7:14 AM
hans accepted this revision.Wed, May 26, 7:21 AM

Looks good to me!

This revision is now accepted and ready to land.Wed, May 26, 7:21 AM

Thanks for the review! Can you take care of merging the patch? I don't have access.

This revision was landed with ongoing or failed builds.Mon, May 31, 1:45 AM
This revision was automatically updated to reflect the committed changes.

Sorry, had to revert it as this fails under sanitizer : https://lab.llvm.org/buildbot/#/builders/5/builds/8150

AbbasSabra reopened this revision.Mon, May 31, 8:17 AM
This revision is now accepted and ready to land.Mon, May 31, 8:17 AM

Updating D102517: [clang]Fix use-of-uninitialized-value detected by the sanitizer

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

AbbasSabra requested review of this revision.Mon, May 31, 8:31 AM
This revision was not accepted when it landed; it landed in state Needs Review.Mon, May 31, 9:50 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
hans added a comment.Mon, May 31, 9:51 AM

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.