[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Stephan Bergmann via Phabricator via cfe-commits]

sberg added a comment.

In D58896#1738288 , @aaron.ballman 
wrote:

> In D58896#1738263 , @sberg wrote:
>
> > In D58896#1737242 , @edward-jones 
> > wrote:
> >
> > > In D58896#1737113 , @sberg wrote:
> > >
> > > > But how about literals like `'\x80'` where the promoted value depends 
> > > > on whether plain `char` is signed or unsigned?
> > >
> > >
> > > If 'char' is signed and index into an array then this will typically 
> > > trigger an `-Warray-bounds` warning because it references before the 
> > > start of the array.
> >
> >
> > My thought was more that it might be useful as a kind of portability 
> > warning.
>
>
> I'm not opposed to the warning per-se, but do you have evidence that the 
> situation occurs in real-world code?


No.  (My original comment was driven by my, potentially false, assumption that 
this warning was originally, at least in part, meant to flag portability 
issues---along the lines of: why else would the warning trigger at all when 
`char` is unsigned.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D58896#1738263 , @sberg wrote:

> In D58896#1737242 , @edward-jones 
> wrote:
>
> > In D58896#1737113 , @sberg wrote:
> >
> > > But how about literals like `'\x80'` where the promoted value depends on 
> > > whether plain `char` is signed or unsigned?
> >
> >
> > If 'char' is signed and index into an array then this will typically 
> > trigger an `-Warray-bounds` warning because it references before the start 
> > of the array.
>
>
> My thought was more that it might be useful as a kind of portability warning.


I'm not opposed to the warning per-se, but do you have evidence that the 
situation occurs in real-world code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Stephan Bergmann via Phabricator via cfe-commits]

sberg added a comment.

In D58896#1737242 , @edward-jones 
wrote:

> In D58896#1737113 , @sberg wrote:
>
> > But how about literals like `'\x80'` where the promoted value depends on 
> > whether plain `char` is signed or unsigned?
>
>
> If 'char' is signed and index into an array then this will typically trigger 
> an `-Warray-bounds` warning because it references before the start of the 
> array.


My thought was more that it might be useful as a kind of portability warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

int('i') is nice fixit we could emit in this case! Thanks for this idea!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

In D58896#1737242 , @edward-jones 
wrote:

> In D58896#1737200 , @xbolva00 wrote:
>
> > Well, i am not sure if one twitter report is good motivation to criple 
> > warning.
>
>
> The motivation for suppressing the warning was that it is not uncommon to use 
> a character literal in lookup tables. In addition in brings the warning 
> inline with the behaviour of `-Wchar-subscripts` in GCC.


Peanut gallery says: If it's "not uncommon," then {when,if} you resubmit this 
for review, it should be easy and therefore mandatory to provide at least one 
example of someone actually using a character literal as a lookup table index. 
I've never seen such a construct in my life, so real-world examples (e.g. 
GitHub links) would be useful.

Especially useful would be links to GitHub examples of `'i'[arr]` or `arr['i']` 
where the //most appropriate fix// is legitimately "upgrade your Clang thus 
suppressing the diagnostic" as opposed to "rewrite the expression as 
`arr[int('i')]` or `arr[105]` depending on what you actually mean it to do."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Edward Jones via Phabricator via cfe-commits]

edward-jones added a comment.

Okay I've reverted this in rG90ecfa2f5f7f 
 . I'll 
make improvements and resubmit this for review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

It was said Clang does not have to match gcc 1:1.

Just because someone uses weird patterns and instead of using pragma push/pop 
diagnostic in his/her code, should we really change it in Clang?

You should provide extra flag which controls this specific behaviour (and on by 
default) so user can use it to disable only this specific case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Edward Jones via Phabricator via cfe-commits]

edward-jones added a comment.

In D58896#1737113 , @sberg wrote:

> But how about literals like `'\x80'` where the promoted value depends on 
> whether plain `char` is signed or unsigned?


If 'char' is signed and index into an array then this will typically trigger an 
`-Warray-bounds` warning because it references before the start of the array.

In D58896#1737200 , @xbolva00 wrote:

> Well, i am not sure if one twitter report is good motivation to criple 
> warning.


The motivation for suppressing the warning was that it is not uncommon to use a 
character literal in lookup tables. In addition in brings the warning inline 
with the behaviour of `-Wchar-subscripts` in GCC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Well, i am not sure if one twitter report is good motivation to criple warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Stephan Bergmann via Phabricator via cfe-commits]

sberg added a comment.

But how about literals like `'\x80'` where the promoted value depends on 
whether plain `char` is signed or unsigned?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Edward Jones via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7adab7719e55: [Sema] Suppress -Wchar-subscripts if the index 
is a literal char (authored by edward-jones).
Herald added a subscriber: simoncook.

Changed prior to commit:
  https://reviews.llvm.org/D58896?vs=189127=228249#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58896

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/warn-char-subscripts.cpp


Index: clang/test/SemaCXX/warn-char-subscripts.cpp
===
--- clang/test/SemaCXX/warn-char-subscripts.cpp
+++ clang/test/SemaCXX/warn-char-subscripts.cpp
@@ -14,6 +14,19 @@
   int val = subscript[array]; // expected-warning{{array subscript is of type 
'char'}}
 }
 
+void t3() {
+  int array[50] = { 0 };
+  int val = array[' ']; // no warning, subscript is a literal
+}
+void t4() {
+  int array[50] = { 0 };
+  int val = '('[array]; // no warning, subscript is a literal
+}
+void t5() {
+  int array[50] = { 0 };
+  int val = array['\x11']; // no warning, subscript is a literal
+}
+
 void test() {
   t1(); // expected-note {{in instantiation of function template 
specialization 't1' requested here}}
   t2(); // expected-note {{in instantiation of function template 
specialization 't2' requested here}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -4741,7 +4741,8 @@
 
   if ((IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_S) ||
IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_U))
- && !IndexExpr->isTypeDependent())
+ && !IndexExpr->isTypeDependent()
+ && !isa(IndexExpr))
 Diag(LLoc, diag::warn_subscript_is_char) << IndexExpr->getSourceRange();
 
   // C99 6.5.2.1p1: "shall have type "pointer to *object* type". Similarly,


Index: clang/test/SemaCXX/warn-char-subscripts.cpp
===
--- clang/test/SemaCXX/warn-char-subscripts.cpp
+++ clang/test/SemaCXX/warn-char-subscripts.cpp
@@ -14,6 +14,19 @@
   int val = subscript[array]; // expected-warning{{array subscript is of type 'char'}}
 }
 
+void t3() {
+  int array[50] = { 0 };
+  int val = array[' ']; // no warning, subscript is a literal
+}
+void t4() {
+  int array[50] = { 0 };
+  int val = '('[array]; // no warning, subscript is a literal
+}
+void t5() {
+  int array[50] = { 0 };
+  int val = array['\x11']; // no warning, subscript is a literal
+}
+
 void test() {
   t1(); // expected-note {{in instantiation of function template specialization 't1' requested here}}
   t2(); // expected-note {{in instantiation of function template specialization 't2' requested here}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -4741,7 +4741,8 @@
 
   if ((IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_S) ||
IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_U))
- && !IndexExpr->isTypeDependent())
+ && !IndexExpr->isTypeDependent()
+ && !isa(IndexExpr))
 Diag(LLoc, diag::warn_subscript_is_char) << IndexExpr->getSourceRange();
 
   // C99 6.5.2.1p1: "shall have type "pointer to *object* type". Similarly,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[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 D58896#1428816 , @edward-jones 
wrote:

> In D58896#1419964 , @aaron.ballman 
> wrote:
>
> > Do you have some evidence that the current behavior is causing a lot of 
> > false positives in the wild? For ASCII character literals, I can sort of 
> > guess at why people might want to do this, but for things like wide 
> > character literals, or character literals relying on the current code page, 
> > etc, I'm less convinced.
>
>
> I don't know about the false positive rate,  just the one report on twitter 
> which triggered me to submit this change. As for wide character literals I 
> was under the impression that they would be promoted to integers and wouldn't 
> have triggered the -Wchar-subscript anyway?


Ordinary character literals are promoted as well, aren't they? `'a'` has type 
`char`, and that should promote up to `int`. My point was that you are 
silencing the warning on any character literal, not just ordinary character 
literals. However, I see now that `-Wchar-subscript` is very serious about 
`char`, rather than any character type, so I guess you don't need to 
distinguish between character literal kinds because the type system already 
deals with that for you.

LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Edward Jones via Phabricator via cfe-commits]

edward-jones added a comment.

In D58896#1419964 , @aaron.ballman 
wrote:

> Do you have some evidence that the current behavior is causing a lot of false 
> positives in the wild? For ASCII character literals, I can sort of guess at 
> why people might want to do this, but for things like wide character 
> literals, or character literals relying on the current code page, etc, I'm 
> less convinced.


I don't know about the false positive rate,  just the one report on twitter 
which triggered me to submit this change. As for wide character literals I was 
under the impression that they would be promoted to integers and wouldn't have 
triggered the -Wchar-subscript anyway?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Do you have some evidence that the current behavior is causing a lot of false 
positives in the wild? For ASCII character literals, I can sort of guess at why 
people might want to do this, but for things like wide character literals, or 
character literals relying on the current code page, etc, I'm less convinced.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58896



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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Edward Jones via Phabricator via cfe-commits]

edward-jones created this revision.
Herald added subscribers: cfe-commits, arphaman.
Herald added a project: clang.

Assume that the user knows what they're doing if they provide a char literal as 
an array index. This more closely matches the behavior of GCC.


Repository:
  rC Clang

https://reviews.llvm.org/D58896

Files:
  lib/Sema/SemaExpr.cpp
  test/SemaCXX/warn-char-subscripts.cpp


Index: test/SemaCXX/warn-char-subscripts.cpp
===
--- test/SemaCXX/warn-char-subscripts.cpp
+++ test/SemaCXX/warn-char-subscripts.cpp
@@ -14,6 +14,19 @@
   int val = subscript[array]; // expected-warning{{array subscript is of type 
'char'}}
 }
 
+void t3() {
+  int array[50] = { 0 };
+  int val = array[' ']; // no warning, subscript is a literal
+}
+void t4() {
+  int array[50] = { 0 };
+  int val = '('[array]; // no warning, subscript is a literal
+}
+void t5() {
+  int array[50] = { 0 };
+  int val = array['\x11']; // no warning, subscript is a literal
+}
+
 void test() {
   t1(); // expected-note {{in instantiation of function template 
specialization 't1' requested here}}
   t2(); // expected-note {{in instantiation of function template 
specialization 't2' requested here}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -4718,7 +4718,8 @@
 
   if ((IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_S) ||
IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_U))
- && !IndexExpr->isTypeDependent())
+ && !IndexExpr->isTypeDependent()
+ && !isa(IndexExpr))
 Diag(LLoc, diag::warn_subscript_is_char) << IndexExpr->getSourceRange();
 
   // C99 6.5.2.1p1: "shall have type "pointer to *object* type". Similarly,


Index: test/SemaCXX/warn-char-subscripts.cpp
===
--- test/SemaCXX/warn-char-subscripts.cpp
+++ test/SemaCXX/warn-char-subscripts.cpp
@@ -14,6 +14,19 @@
   int val = subscript[array]; // expected-warning{{array subscript is of type 'char'}}
 }
 
+void t3() {
+  int array[50] = { 0 };
+  int val = array[' ']; // no warning, subscript is a literal
+}
+void t4() {
+  int array[50] = { 0 };
+  int val = '('[array]; // no warning, subscript is a literal
+}
+void t5() {
+  int array[50] = { 0 };
+  int val = array['\x11']; // no warning, subscript is a literal
+}
+
 void test() {
   t1(); // expected-note {{in instantiation of function template specialization 't1' requested here}}
   t2(); // expected-note {{in instantiation of function template specialization 't2' requested here}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -4718,7 +4718,8 @@
 
   if ((IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_S) ||
IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_U))
- && !IndexExpr->isTypeDependent())
+ && !IndexExpr->isTypeDependent()
+ && !isa(IndexExpr))
 Diag(LLoc, diag::warn_subscript_is_char) << IndexExpr->getSourceRange();
 
   // C99 6.5.2.1p1: "shall have type "pointer to *object* type". Similarly,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits