Page MenuHomePhabricator

[clang] Fix for "Bug 27113 - MSVC-compat __identifier implementation incomplete"
ClosedPublic

Authored by super_concat on Apr 10 2021, 6:27 PM.

Details

Summary

this patch fixes Bug 27113 by adding support for string literals to the implementation of the MS extension __identifier.

Diff Detail

Event Timeline

super_concat requested review of this revision.Apr 10 2021, 6:27 PM
super_concat created this revision.
super_concat retitled this revision from Fix for "Bug 27113 - MSVC-compat __identifier implementation incomplete" to [clang] Fix for "Bug 27113 - MSVC-compat __identifier implementation incomplete".Apr 21 2021, 2:25 PM
rsmith edited reviewers, added: rnk, hans; removed: rsmith.May 18 2021, 5:52 PM
hans added a comment.May 19 2021, 5:22 AM

Interesting, I hadn't seen __identifier before. It seems like a pretty esoteric feature.

clang/lib/Lex/PPMacroExpansion.cpp
1817

I'm not sure this is the right way to get the string out of a string_literal.. this pattern seems more common: https://github.com/llvm/llvm-project/blob/llvmorg-12.0.0/clang/lib/Lex/Pragma.cpp#L755

For example, what should happen with __identifier("foo" "bar")?

1819

It says in the bug report that MSVC emits a warning for this. What does the warning look like, and would it make sense for Clang too warn too (it might not, just wondering if we should consider it).

clang/test/Parser/MicrosoftExtensions.cpp
273

Maybe instead of removing this, move it to the cases that work above. For example, I guess this should work now?

int __identifier("foo") = 42;
int bar = foo;
276

It would be interesting to see a non-error test case too, perhaps with a mangled name like in the PR, and to see that it makes it through to LLVM IR correctly..

super_concat updated this revision to Diff 346595.EditedMay 19 2021, 5:25 PM

Correctly handle string literals and add more test cases. Also MSVC by default gives an error when trying to use __identifier with a string literal but disabling/suppressing that error allows string literals to be used with __identifier. However given how esoteric it is, it wouldn't really be necessary for clang to give a warning or error like MSVC

hans accepted this revision.May 20 2021, 4:46 AM

lgtm

This revision is now accepted and ready to land.May 20 2021, 4:46 AM
super_concat marked 3 inline comments as done.May 20 2021, 12:48 PM

@hans, could you land this for me? I do not have commit access.

This revision was landed with ongoing or failed builds.May 21 2021, 2:14 AM
This revision was automatically updated to reflect the committed changes.