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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

In D87962#2313363 , @nomanous wrote:

> In D87962#2312617 , @aaron.ballman 
> wrote:
>
>> LGTM! Do you need someone to commit on your behalf?
>
> Yes I need!

I've committed on your behalf in 8fa45e1fd527269140c4e2a1652fef5500da16fd 
, thank 
you for the fix!


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

https://reviews.llvm.org/D87962

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuyang Chen via Phabricator via cfe-commits]

nomanous added a comment.

In D87962#2312617 , @aaron.ballman 
wrote:

> LGTM! Do you need someone to commit on your behalf?

Yes I need!


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

https://reviews.llvm.org/D87962

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM! Do you need someone to commit on your behalf?


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

https://reviews.llvm.org/D87962

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuyang Chen via Phabricator via cfe-commits]

nomanous updated this revision to Diff 296118.
nomanous added a comment.

In this revision I delete the useless main function in the new test case and 
rename ext_multichar_character_literal & ext_four_char_character_literal to 
warn_multichar_character_literal & warn_four_char_character_literal.


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

https://reviews.llvm.org/D87962

Files:
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/lib/Lex/LiteralSupport.cpp
  clang/test/Lexer/constants.c
  clang/test/Lexer/multi-char-constants.c


Index: clang/test/Lexer/multi-char-constants.c
===
--- /dev/null
+++ clang/test/Lexer/multi-char-constants.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wfour-char-constants 
-pedantic-errors %s
+
+int x = 'ab'; // expected-warning {{multi-character character constant}}
+int y = 'abcd'; // expected-warning {{multi-character character constant}}
Index: clang/test/Lexer/constants.c
===
--- clang/test/Lexer/constants.c
+++ clang/test/Lexer/constants.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -pedantic -ftrigraphs %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wfour-char-constants -pedantic 
-ftrigraphs %s
 
 int x = 00080;  // expected-error {{invalid digit}}
 
Index: clang/lib/Lex/LiteralSupport.cpp
===
--- clang/lib/Lex/LiteralSupport.cpp
+++ clang/lib/Lex/LiteralSupport.cpp
@@ -1373,9 +1373,9 @@
 if (isWide())
   PP.Diag(Loc, diag::warn_extraneous_char_constant);
 else if (isAscii() && NumCharsSoFar == 4)
-  PP.Diag(Loc, diag::ext_four_char_character_literal);
+  PP.Diag(Loc, diag::warn_four_char_character_literal);
 else if (isAscii())
-  PP.Diag(Loc, diag::ext_multichar_character_literal);
+  PP.Diag(Loc, diag::warn_multichar_character_literal);
 else
   PP.Diag(Loc, diag::err_multichar_utf_character_literal);
 IsMultiChar = true;
Index: clang/include/clang/Basic/DiagnosticLexKinds.td
===
--- clang/include/clang/Basic/DiagnosticLexKinds.td
+++ clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -104,10 +104,10 @@
   "raw string literals are incompatible with C++98">,
   InGroup, DefaultIgnore;
 
-def ext_multichar_character_literal : ExtWarn<
+def warn_multichar_character_literal : Warning<
   "multi-character character constant">, InGroup;
-def ext_four_char_character_literal : Extension<
-  "multi-character character constant">, InGroup;
+def warn_four_char_character_literal : Warning<
+  "multi-character character constant">, InGroup, 
DefaultIgnore;
 
 
 // Unicode and UCNs


Index: clang/test/Lexer/multi-char-constants.c
===
--- /dev/null
+++ clang/test/Lexer/multi-char-constants.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wfour-char-constants -pedantic-errors %s
+
+int x = 'ab'; // expected-warning {{multi-character character constant}}
+int y = 'abcd'; // expected-warning {{multi-character character constant}}
Index: clang/test/Lexer/constants.c
===
--- clang/test/Lexer/constants.c
+++ clang/test/Lexer/constants.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -pedantic -ftrigraphs %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wfour-char-constants -pedantic -ftrigraphs %s
 
 int x = 00080;  // expected-error {{invalid digit}}
 
Index: clang/lib/Lex/LiteralSupport.cpp
===
--- clang/lib/Lex/LiteralSupport.cpp
+++ clang/lib/Lex/LiteralSupport.cpp
@@ -1373,9 +1373,9 @@
 if (isWide())
   PP.Diag(Loc, diag::warn_extraneous_char_constant);
 else if (isAscii() && NumCharsSoFar == 4)
-  PP.Diag(Loc, diag::ext_four_char_character_literal);
+  PP.Diag(Loc, diag::warn_four_char_character_literal);
 else if (isAscii())
-  PP.Diag(Loc, diag::ext_multichar_character_literal);
+  PP.Diag(Loc, diag::warn_multichar_character_literal);
 else
   PP.Diag(Loc, diag::err_multichar_utf_character_literal);
 IsMultiChar = true;
Index: clang/include/clang/Basic/DiagnosticLexKinds.td
===
--- clang/include/clang/Basic/DiagnosticLexKinds.td
+++ clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -104,10 +104,10 @@
   "raw string literals are incompatible with C++98">,
   InGroup, DefaultIgnore;
 
-def ext_multichar_character_literal : ExtWarn<
+def warn_multichar_character_literal : Warning<
   "multi-character character constant">, InGroup;
-def ext_four_char_character_literal : Extension<
-  "multi-character character constant">, InGroup;
+def warn_four_char_character_literal : Warning<
+  "multi-character character c

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:109
   "multi-character character constant">, InGroup;
-def ext_four_char_character_literal : Extension<
+def ext_four_char_character_literal : Warning<
   "multi-character character constant">, InGroup;

aaron.ballman wrote:
> One potential reason why we don't want to warn on this by default is that 
> four char codes were quite popular back in the Mac Classic days.
Please also rename these diagnostics from ext_ to warn_.


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

https://reviews.llvm.org/D87962

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D87962#2307824 , @rsmith wrote:

> In D87962#2306043 , @aaron.ballman 
> wrote:
>
>> 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.
>
> To be clear: that's how they happen to most visibly be used, but the more 
> general idea is that Remarks are for purely informational messages. One test 
> for whether a diagnostic could reasonably be a remark is: can we imagine 
> anyone ever reasonably wanting to promote it to an error as part of the flags 
> for some project? If so, then use of a remark is inappropriate. That's 
> certainly the case here: it's entirely reasonable to want to be able to 
> reject use of non-portable functionality such as this. So I agree, this 
> should not be a remark.

Thank you for the explanation!

The patch LGTM with a minor nit about the test case.




Comment at: clang/test/Lexer/multi-char-constants.c:6
+
+int main() {
+   return 0;

You can remove the empty `main()`.


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

https://reviews.llvm.org/D87962

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuyang Chen via Phabricator via cfe-commits]

nomanous updated this revision to Diff 295771.
nomanous added a comment.

I restore some needless modification in the previous patch.


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

https://reviews.llvm.org/D87962

Files:
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/test/Lexer/constants.c
  clang/test/Lexer/multi-char-constants.c


Index: clang/test/Lexer/multi-char-constants.c
===
--- /dev/null
+++ clang/test/Lexer/multi-char-constants.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wfour-char-constants 
-pedantic-errors %s
+
+int x = 'ab'; // expected-warning {{multi-character character constant}}
+int y = 'abcd'; // expected-warning {{multi-character character constant}}
+
+int main() {
+   return 0;
+}
+
Index: clang/test/Lexer/constants.c
===
--- clang/test/Lexer/constants.c
+++ clang/test/Lexer/constants.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -pedantic -ftrigraphs %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wfour-char-constants -pedantic 
-ftrigraphs %s
 
 int x = 00080;  // expected-error {{invalid digit}}
 
Index: clang/include/clang/Basic/DiagnosticLexKinds.td
===
--- clang/include/clang/Basic/DiagnosticLexKinds.td
+++ clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -104,10 +104,10 @@
   "raw string literals are incompatible with C++98">,
   InGroup, DefaultIgnore;
 
-def ext_multichar_character_literal : ExtWarn<
+def ext_multichar_character_literal : Warning<
   "multi-character character constant">, InGroup;
-def ext_four_char_character_literal : Extension<
-  "multi-character character constant">, InGroup;
+def ext_four_char_character_literal : Warning<
+  "multi-character character constant">, InGroup, 
DefaultIgnore;
 
 
 // Unicode and UCNs


Index: clang/test/Lexer/multi-char-constants.c
===
--- /dev/null
+++ clang/test/Lexer/multi-char-constants.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wfour-char-constants -pedantic-errors %s
+
+int x = 'ab'; // expected-warning {{multi-character character constant}}
+int y = 'abcd'; // expected-warning {{multi-character character constant}}
+
+int main() {
+   return 0;
+}
+
Index: clang/test/Lexer/constants.c
===
--- clang/test/Lexer/constants.c
+++ clang/test/Lexer/constants.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -pedantic -ftrigraphs %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wfour-char-constants -pedantic -ftrigraphs %s
 
 int x = 00080;  // expected-error {{invalid digit}}
 
Index: clang/include/clang/Basic/DiagnosticLexKinds.td
===
--- clang/include/clang/Basic/DiagnosticLexKinds.td
+++ clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -104,10 +104,10 @@
   "raw string literals are incompatible with C++98">,
   InGroup, DefaultIgnore;
 
-def ext_multichar_character_literal : ExtWarn<
+def ext_multichar_character_literal : Warning<
   "multi-character character constant">, InGroup;
-def ext_four_char_character_literal : Extension<
-  "multi-character character constant">, InGroup;
+def ext_four_char_character_literal : Warning<
+  "multi-character character constant">, InGroup, DefaultIgnore;
 
 
 // Unicode and UCNs
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuyang Chen via Phabricator via cfe-commits]

nomanous updated this revision to Diff 295766.
nomanous added a comment.

I change the four-char constants back to "Warning" in this revision, but set it 
to be ignored by default.


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

https://reviews.llvm.org/D87962

Files:
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/test/FixIt/format.m
  clang/test/Lexer/constants.c
  clang/test/Lexer/multi-char-constants.c


Index: clang/test/Lexer/multi-char-constants.c
===
--- /dev/null
+++ clang/test/Lexer/multi-char-constants.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wfour-char-constants 
-pedantic-errors %s
+
+int x = 'ab'; // expected-warning {{multi-character character constant}}
+int y = 'abcd'; // expected-warning {{multi-character character constant}}
+
+int main() {
+   return 0;
+}
+
Index: clang/test/Lexer/constants.c
===
--- clang/test/Lexer/constants.c
+++ clang/test/Lexer/constants.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -pedantic -ftrigraphs %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wfour-char-constants -pedantic 
-ftrigraphs %s
 
 int x = 00080;  // expected-error {{invalid digit}}
 
@@ -26,7 +26,7 @@
   '\\
 t',
   '??!',  // expected-warning {{trigraph converted to '|' character}}
-  'abcd'  // expected-warning {{multi-character character constant}}
+  'abcd' // expected-warning {{multi-character character constant}}
 };
 
 //  PR4499
@@ -36,15 +36,14 @@
 int m3 = '\\\
 ';
 
-
 #pragma clang diagnostic ignored "-Wmultichar"
 
 int d = 'df'; // no warning.
-int e = 'abcd';  // still warn: expected-warning {{multi-character character 
constant}}
+int e = 'abcd'; // still warn: expected-warning {{multi-character character 
constant}}
 
 #pragma clang diagnostic ignored "-Wfour-char-constants"
 
-int f = 'abcd';  // ignored.
+int f = 'abcd'; // ignored.
 
 // rdar://problem/6974641
 float t0[] = {
Index: clang/test/FixIt/format.m
===
--- clang/test/FixIt/format.m
+++ clang/test/FixIt/format.m
@@ -177,7 +177,7 @@
   // type-checker expects %c to correspond to an integer argument, because
   // many C library functions like fgetc() actually return an int (using -1
   // as a sentinel).
-  NSLog(@"%c", 'abcd'); // missing-warning{{format specifies type 'char' but 
the argument has type 'int'}}
+  NSLog(@"%c", 'abcd'); // missing-warning{{format specifies type 'char' but 
the argument has type 'int'}} \
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%d"
 }
 
Index: clang/include/clang/Basic/DiagnosticLexKinds.td
===
--- clang/include/clang/Basic/DiagnosticLexKinds.td
+++ clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -104,10 +104,10 @@
   "raw string literals are incompatible with C++98">,
   InGroup, DefaultIgnore;
 
-def ext_multichar_character_literal : ExtWarn<
+def ext_multichar_character_literal : Warning<
   "multi-character character constant">, InGroup;
-def ext_four_char_character_literal : Extension<
-  "multi-character character constant">, InGroup;
+def ext_four_char_character_literal : Warning<
+  "multi-character character constant">, InGroup, 
DefaultIgnore;
 
 
 // Unicode and UCNs


Index: clang/test/Lexer/multi-char-constants.c
===
--- /dev/null
+++ clang/test/Lexer/multi-char-constants.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wfour-char-constants -pedantic-errors %s
+
+int x = 'ab'; // expected-warning {{multi-character character constant}}
+int y = 'abcd'; // expected-warning {{multi-character character constant}}
+
+int main() {
+   return 0;
+}
+
Index: clang/test/Lexer/constants.c
===
--- clang/test/Lexer/constants.c
+++ clang/test/Lexer/constants.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -pedantic -ftrigraphs %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wfour-char-constants -pedantic -ftrigraphs %s
 
 int x = 00080;  // expected-error {{invalid digit}}
 
@@ -26,7 +26,7 @@
   '\\
 t',
   '??!',  // expected-warning {{trigraph converted to '|' character}}
-  'abcd'  // expected-warning {{multi-character character constant}}
+  'abcd' // expected-warning {{multi-character character constant}}
 };
 
 //  PR4499
@@ -36,15 +36,14 @@
 int m3 = '\\\
 ';
 
-
 #pragma clang diagnostic ignored "-Wmultichar"
 
 int d = 'df'; // no warning.
-int e = 'abcd';  // still warn: expected-warning {{multi-character character constant}}
+int e = 'abcd'; // still warn: expected-warning {{multi-character character constant}}
 
 #pragma clang diagnostic ignored "-Wfour-char-constants"
 
-int f = 'abcd';  // ignored.
+int f = 'abcd'; // ignored.
 
 // rdar://problem/6974641
 float t0[] = {
Index

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D87962#2306043 , @aaron.ballman 
wrote:

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

To be clear: that's how they happen to most visibly be used, but the more 
general idea is that Remarks are for purely informational messages. One test 
for whether a diagnostic could reasonably be a remark is: can we imagine anyone 
ever reasonably wanting to promote it to an error as part of the flags for some 
project? If so, then use of a remark is inappropriate. That's certainly the 
case here: it's entirely reasonable to want to be able to reject use of 
non-portable functionality such as this. So I agree, this should not be a 
remark.

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

Please also ensure you upload a diff with full context 
.


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

https://reviews.llvm.org/D87962

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

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

In D87962#2305375 , @nomanous wrote:

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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

There's a DefaultIgnore modifier for warnings that turns then off by default.  
We use this sparingly, since it's hard to discover off-by-default warnings, but 
seems appropriate here.


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

https://reviews.llvm.org/D87962

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuyang Chen via Phabricator via cfe-commits]

nomanous updated this revision to Diff 295559.

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

https://reviews.llvm.org/D87962

Files:
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/test/CXX/lex/lex.literal/lex.ccon/p1.cpp
  clang/test/FixIt/format.m
  clang/test/Lexer/char-literal.cpp
  clang/test/Lexer/constants.c
  clang/test/Lexer/multi-character-character-constant.c
  clang/test/Preprocessor/expr_multichar.c

Index: clang/test/Preprocessor/expr_multichar.c
===
--- clang/test/Preprocessor/expr_multichar.c
+++ clang/test/Preprocessor/expr_multichar.c
@@ -1,6 +1,6 @@
 // RUN: %clang_cc1 < %s -E -verify -triple i686-pc-linux-gnu
-// Expect the warning of the four-character character constant after changing it from extension to implementation-defined
+// expected-no-diagnostics
 
-#if (('1234' >> 24) != '1') // expected-warning {{multi-character character constant}}
+#if (('1234' >> 24) != '1')
 #error Bad multichar constant calculation!
 #endif
Index: clang/test/Lexer/multi-character-character-constant.c
===
--- clang/test/Lexer/multi-character-character-constant.c
+++ clang/test/Lexer/multi-character-character-constant.c
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -pedantic-errors %s
 
 int x = 'ab'; // expected-warning {{multi-character character constant}}
-int y = 'abcd'; // expected-warning {{multi-character character constant}}
+int y = 'abcd'; // no warning.
 
 int main() {
return 0;
Index: clang/test/Lexer/constants.c
===
--- clang/test/Lexer/constants.c
+++ clang/test/Lexer/constants.c
@@ -26,7 +26,7 @@
   '\\
 t',
   '??!',  // expected-warning {{trigraph converted to '|' character}}
-  'abcd'  // expected-warning {{multi-character character constant}}
+  'abcd'
 };
 
 //  PR4499
@@ -36,15 +36,16 @@
 int m3 = '\\\
 ';
 
+// The four-character character constants wouldn't cause warning anymore after we change it to "Remark"
 
 #pragma clang diagnostic ignored "-Wmultichar"
 
 int d = 'df'; // no warning.
-int e = 'abcd';  // still warn: expected-warning {{multi-character character constant}}
+int e = 'abcd';
 
-#pragma clang diagnostic ignored "-Wfour-char-constants"
+// #pragma clang diagnostic ignored "-Wfour-char-constants"
 
-int f = 'abcd';  // ignored.
+int f = 'abcd';
 
 // rdar://problem/6974641
 float t0[] = {
Index: clang/test/Lexer/char-literal.cpp
===
--- clang/test/Lexer/char-literal.cpp
+++ clang/test/Lexer/char-literal.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -Wfour-char-constants -fsyntax-only -verify %s
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c11 -x c -Wfour-char-constants -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c11 -x c -fsyntax-only -verify %s
 
 #ifndef __cplusplus
 typedef __WCHAR_TYPE__ wchar_t;
@@ -9,7 +9,7 @@
 
 int a = 'ab'; // expected-warning {{multi-character character constant}}
 int b = '\xFF\xFF'; // expected-warning {{multi-character character constant}}
-int c = 'APPS'; // expected-warning {{multi-character character constant}}
+int c = 'APPS';
 
 char d = '鈱?; // expected-error {{character too large for enclosing character literal type}}
 char e = '\u2318'; // expected-error {{character too large for enclosing character literal type}}
Index: clang/test/FixIt/format.m
===
--- clang/test/FixIt/format.m
+++ clang/test/FixIt/format.m
@@ -162,16 +162,13 @@
 
 
   NSLog(@"%s", 'abcd'); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}} \
-  // expected-warning {{multi-character character constant}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:11-[[@LINE-2]]:13}:"%d"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%d"
 
   NSLog(@"%lf", 'abcd'); // expected-warning{{format specifies type 'double' but the argument has type 'int'}} \
-  // expected-warning {{multi-character character constant}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:11-[[@LINE-2]]:14}:"%d"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:14}:"%d"
 
   NSLog(@"%@", 'abcd'); // expected-warning{{format specifies type 'id' but the argument has type 'int'}} \
-  // expected-warning {{multi-character character constant}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:11-[[@LINE-2]]:13}:"%d"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%d"
 }
 
 void multichar_constants_false_negative() {
@@ -181,8 +178,7 @@
   // many C library functions like fgetc() actually return an int (using -1
   // as a sentinel).
   NSLog(@"%c", 'abcd'); // missing-warning{{format specifies type 'char' but the argument has type 'int'}}

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

[Chuyang Chen via Phabricator via cfe-commits]

nomanous added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87962

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.




Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:109
   "multi-character character constant">, InGroup;
-def ext_four_char_character_literal : Extension<
+def ext_four_char_character_literal : Warning<
   "multi-character character constant">, InGroup;

One potential reason why we don't want to warn on this by default is that four 
char codes were quite popular back in the Mac Classic days.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87962

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuyang Chen via Phabricator via cfe-commits]

nomanous added a comment.

Ping @rsmith


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87962

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuyang Chen via Phabricator via cfe-commits]

nomanous created this revision.
nomanous added a reviewer: rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
nomanous requested review of this revision.

The multi-character character constants should be implementation-defined 
according to the C standard but actually were made to be extension, which would 
cause errors when using clang with option "-pedantic-errors". This patch  fixes 
it and it is for bug #46797 on the Bugzilla system.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87962

Files:
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/test/CXX/lex/lex.literal/lex.ccon/p1.cpp
  clang/test/FixIt/format.m
  clang/test/Lexer/multi-character-character-constant.c
  clang/test/Preprocessor/expr_multichar.c

Index: clang/test/Preprocessor/expr_multichar.c
===
--- clang/test/Preprocessor/expr_multichar.c
+++ clang/test/Preprocessor/expr_multichar.c
@@ -1,6 +1,6 @@
 // RUN: %clang_cc1 < %s -E -verify -triple i686-pc-linux-gnu
-// expected-no-diagnostics
+// Expect the warning of the four-character character constant after changing it from extension to implementation-defined
 
-#if (('1234' >> 24) != '1')
+#if (('1234' >> 24) != '1') // expected-warning {{multi-character character constant}}
 #error Bad multichar constant calculation!
 #endif
Index: clang/test/Lexer/multi-character-character-constant.c
===
--- /dev/null
+++ clang/test/Lexer/multi-character-character-constant.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -pedantic-errors %s
+
+int x = 'ab'; // expected-warning {{multi-character character constant}}
+int y = 'abcd'; // expected-warning {{multi-character character constant}}
+
+int main() {
+   return 0;
+}
+
Index: clang/test/FixIt/format.m
===
--- clang/test/FixIt/format.m
+++ clang/test/FixIt/format.m
@@ -161,14 +161,17 @@
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
 
 
-  NSLog(@"%s", 'abcd'); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%d"
+  NSLog(@"%s", 'abcd'); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}} \
+  // expected-warning {{multi-character character constant}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:11-[[@LINE-2]]:13}:"%d"
 
-  NSLog(@"%lf", 'abcd'); // expected-warning{{format specifies type 'double' but the argument has type 'int'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:14}:"%d"
+  NSLog(@"%lf", 'abcd'); // expected-warning{{format specifies type 'double' but the argument has type 'int'}} \
+  // expected-warning {{multi-character character constant}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:11-[[@LINE-2]]:14}:"%d"
 
-  NSLog(@"%@", 'abcd'); // expected-warning{{format specifies type 'id' but the argument has type 'int'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%d"
+  NSLog(@"%@", 'abcd'); // expected-warning{{format specifies type 'id' but the argument has type 'int'}} \
+  // expected-warning {{multi-character character constant}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:11-[[@LINE-2]]:13}:"%d"
 }
 
 void multichar_constants_false_negative() {
@@ -177,8 +180,9 @@
   // type-checker expects %c to correspond to an integer argument, because
   // many C library functions like fgetc() actually return an int (using -1
   // as a sentinel).
-  NSLog(@"%c", 'abcd'); // missing-warning{{format specifies type 'char' but the argument has type 'int'}}
-  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%d"
+  NSLog(@"%c", 'abcd'); // missing-warning{{format specifies type 'char' but the argument has type 'int'}} \
+  // expected-warning {{multi-character character constant}}
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:11-[[@LINE-2]]:13}:"%d"
 }
 
 
Index: clang/test/CXX/lex/lex.literal/lex.ccon/p1.cpp
===
--- clang/test/CXX/lex/lex.literal/lex.ccon/p1.cpp
+++ clang/test/CXX/lex/lex.literal/lex.ccon/p1.cpp
@@ -1,12 +1,12 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
-// expected-no-diagnostics
+// Expect the warning of the four-character character constant after changing it from extension to implementation-defined
 
 // Check types of char literals
 extern char a;
 extern __typeof('a') a;
 extern int b;
-extern __typeof('asdf') b;
+extern __typeof('asdf') b; // expected-warning {{multi-character character constant}}
 extern wchar_t c;
 extern __typeof(L'a') c;
 #if __cplusplus >= 201103L
Index: clang/include/clang/Basic/DiagnosticLexKinds.td
===
--- clang/include/clang/Basic/DiagnosticLexKinds.td
+++ clang/include/c