Suppress -Wchar-subscripts if the index is a literal char

[edward-jones]

Summary

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.

[aaron.ballman]

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.

[edward-jones]

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?

[aaron.ballman]

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!

[sberg]

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

[xbolva00]

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

[edward-jones]

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.

[xbolva00]

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.

[edward-jones]

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

[xbolva00]

Thank you!

[• Quuxplusone]

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

[xbolva00]

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

[sberg]

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.

[aaron.ballman]

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?

[sberg]

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

clang/lib/Sema/SemaExpr.cpp

Sema::CreateBuiltinArraySubscriptExpr(Expr *Base, SourceLocation LLoc,
   }
   // C99 6.5.2.1p1
   if (!IndexExpr->getType()->isIntegerType() && !IndexExpr->isTypeDependent())
     return ExprError(Diag(LLoc, diag::err_typecheck_subscript_not_integer)
                      << IndexExpr->getSourceRange());
 
   if ((IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_S) ||
        IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_U))
-         && !IndexExpr->isTypeDependent())
+         && !IndexExpr->isTypeDependent()
+         && !isa<CharacterLiteral>(IndexExpr))
     Diag(LLoc, diag::warn_subscript_is_char) << IndexExpr->getSourceRange();
 
   // C99 6.5.2.1p1: "shall have type "pointer to *object* type". Similarly,
   // C++ [expr.sub]p1: The type "T" shall be a completely-defined object
   // type. Note that Functions are not objects, and that (in C99 parlance)
   // incomplete types are not object types.
   if (ResultType->isFunctionType()) {
     Diag(BaseExpr->getBeginLoc(), diag::err_subscript_function_type)

clang/test/SemaCXX/warn-char-subscripts.cpp

 // RUN: %clang_cc1 -Wchar-subscripts -fsyntax-only -verify %s
 
 template<typename T>
 void t1() {
   int array[1] = { 0 };
   T subscript = 0;
   int val = array[subscript]; // expected-warning{{array subscript is of type 'char'}}
 }
 
 template<typename T>
 void t2() {
   int array[1] = { 0 };
   T subscript = 0;
   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<char>(); // expected-note {{in instantiation of function template specialization 't1<char>' requested here}}
   t2<char>(); // expected-note {{in instantiation of function template specialization 't2<char>' requested here}}
 }