This is an archive of the discontinued LLVM Phabricator instance.

Add PragmaHandler for MSVC pragma execution_character_set
ClosedPublic

Authored by sigatrev on Feb 21 2019, 4:10 PM.

Details

Summary

__pragma(execution_character_set(push, "UTF-8")) is used in TraceLoggingProvider.h. This commit
implements a no-op handler for compatability, similar to how the flag -fexec_charset is handled.

Diff Detail

Repository
rL LLVM

Event Timeline

sigatrev created this revision.Feb 21 2019, 4:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 4:10 PM
rnk added a comment.Feb 25 2019, 6:43 PM

Looks good and thorough, but it needs tests.

sigatrev updated this revision to Diff 189629.Mar 6 2019, 5:42 PM

Added tests, and slightly improved associated messaging.

rnk accepted this revision.Mar 7 2019, 10:44 AM

lgtm, thanks! Would you like someone to land this for you?

This revision is now accepted and ready to land.Mar 7 2019, 10:44 AM

Yeah, that would be great. Thanks!

thakis added a subscriber: thakis.Mar 7 2019, 12:52 PM

Does our serialization machinery serialize these correctly automatically? What happens if you compiler a header saying execution_character_set_push, "utf-8") in a header, compile that to a pch, then include that through the pch in a .c file that does pop?

My guess is that this either needs (de)serialization code or a warning when the stack is not empty when a pch is written.

(Examples: https://reviews.llvm.org/rL262493 https://reviews.llvm.org/rL262539 -- the latter one is probably a better example)

This implementation doesn't track the push/pop stack, it just verifies the synax is valid and moves on. I modeled it after the PragmaWarningHandler which does the same, and thought it would be fine in this case since the only accepted value is a no-op.

thakis accepted this revision.Mar 8 2019, 12:18 PM

Ah ok, then I agree this doesn't need serialization yet :)

It'd be nice to at least diag that the stack use is valid (also for the warning stack), but it doesn't have to be in this change.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2019, 11:11 AM