Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

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

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



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.


I'm not sure this is the right way to get the string out of a string_literal.. this pattern seems more common:

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


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


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;

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


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.