[PATCH] D87962: [clang] Change the multi-character character constants from extension to implementation-defined.

Thu Oct 1 07:30:27 PDT 2020

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

In D87962#2305375 <https://reviews.llvm.org/D87962#2305375>, @nomanous wrote:

> In D87962#2298021 <https://reviews.llvm.org/D87962#2298021>, @aaron.ballman wrote:
>
>> Multichar literals are implementation-defined in C and conditionally supported with implementation-defined semantics in C++. I agree that it may make sense to warn about their use for portability reasons, but I'm not certain whether it makes sense to promote their use to be always-on diagnostics. I'm curious to know if this change causes any issues with system headers (which may or may not still define four char codes) or popular libraries.
>>
>> I was curious as to why this was an extension in the first place and found the original commits (https://github.com/llvm/llvm-project/commit/74c95e20af4838152a63010292d1063835176711 and https://github.com/llvm/llvm-project/commit/8577f62622d50183c7413d7507ec783d3c1486fc) but there's no justification as to why this was picked as an extension.
>
> Or should we change the four character case to "Remark" so it wouldn't be warned even with the "-pandemic" option? There seems no other choice.

That doesn't sound like the right approach to me -- Remarks are usually for reporting backend decision-making to the frontend for things like optimization passes. In this case, it may make more sense to make it a `Warning<>` but make it `DefaultIgnore` so that you have to enable it explicitly.

Btw, it looks like when you generated this patch, it was generated against the previous diff and not trunk, so the diff view is misleading. When you update the patch, can you be sure to diff against the trunk?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87962/new/

https://reviews.llvm.org/D87962