Unnecessary warning: "array subscript is of type 'char' [-Wchar-subscripts]" with constant char indexes

[llvmbot]

Bugzilla Link	18389
Version	unspecified
OS	Linux
Reporter	LLVM Bugzilla Contributor
CC	@DougGregor

Extended Description

The following code for example generates the warning:

BackupQueries.cpp:2092:29: warning: array subscript is of type 'char' [-Wchar-subscripts]
bool MachineReadable = opts['m'];

Because 'm' is in ASCII, and therefore a positive char value, on all platforms that GCC and LLVM support and almost certainly on all platforms implementing C++11, this warning seems unnecessary, and clearing it requires obfuscating the code thus:

    bool MachineReadable = opts[(unsigned char)'m'];

We could probably choose not to show this warning when the value of the index is known by the compiler to be positive.

[llvmbot]

Also, I encountered this warning when I use the toupper function (which was implemented using a lookup table, without first casting the subscript to int).

I think pre-known positive constant values should be clear of this warning, and toupper and tolower macros should add casts.

[wheatman]

This behavior is still the same on post 17 trunk(7d495d6)
https://godbolt.org/z/f77G46rWY

code

char upper[300];

int main() {
    upper['a'] = 'A';
    return 1;
}

warning

<source>:4:10: warning: array subscript is of type 'char' [-Wchar-subscripts]
    4 |     upper['a'] = 'A';
      |          ^~~~
1 warning generated.
Compiler returned: 0

[Endilll]

@wheatman I think you have a copy-paste mistake in the previous comment

[shafik]

I think this is a reasonable diagnostic. I think it is highly likely to be a mistake to use a char to index into an array.

CC @AaronBallman

[wheatman]

I think the argument is does this reasoning still hold true when the char is a fixed positive constant, for which I have no opinion.

I did not comment specifically on this post in support of the changed diagnostic, I am simply going through all old issues to see what still has the same behavior years down the road and what has long been fixed and can be closed

[AaronBallman]

I think this is a valid request for QoI. Worth noting that GCC implemented this between 4.1 and 4.4: https://godbolt.org/z/q9aec3Gh1

I think this is a reasonable good first issue. I would tackle this by modifying:

llvm-project/clang/lib/Sema/SemaExpr.cpp

Line 6018 in 64d78d8

if ((IndexExpr->getType()->isSpecificBuiltinType(BuiltinType::Char_S) ||

to look test whether the subscript expression is a constant expression; if it has a known positive value, then we can suppress the warning. We can either tie this to being a character literal explicitly, or we could tie it to any constant value (e.g., constexpr char c = 'a'; array[c];)

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: None (llvmbot)

| | | | --- | --- | | Bugzilla Link | [18389](https://llvm.org/bz18389) | | Version | unspecified | | OS | Linux | | Reporter | LLVM Bugzilla Contributor | | CC | @DougGregor |

Extended Description

The following code for example generates the warning:

BackupQueries.cpp:2092:29: warning: array subscript is of type 'char' [-Wchar-subscripts]
bool MachineReadable = opts['m'];

Because 'm' is in ASCII, and therefore a positive char value, on all platforms that GCC and LLVM support and almost certainly on all platforms implementing C++11, this warning seems unnecessary, and clearing it requires obfuscating the code thus:

    bool MachineReadable = opts[(unsigned char)'m'];

We could probably choose not to show this warning when the value of the index is known by the compiler to be positive.

[wheatman]

In implementing a fix for this I noticed something interesting in the different behaviors between c and c++.
For the following code

void t13(void) {
  int array[256] = { 0 };
  const char b = 'a';
  int val = array[b];
}

If this is c then b is not considered a constant expression and my current implantations with still have the warning.
However, if this is c++ then b is a constant expression and the warning will be suppressed.

In this context I am just using

llvm-project/clang/include/clang/AST/Expr.h

Line 544 in 80737d2

getIntegerConstantExpr(const ASTContext &Ctx,

to define what a constant expression is

[AaronBallman]

In implementing a fix for this I noticed something interesting in the different behaviors between c and c++. For the following code

void t13(void) {
  int array[256] = { 0 };
  const char b = 'a';
  int val = array[b];
}

If this is c then b is not considered a constant expression and my current implantations with still have the warning. However, if this is c++ then b is a constant expression and the warning will be suppressed.

Correct; C has different ideas about constant expressions from C++ (though C23 did get constexpr so there's some light at the end of the tunnel; we don't yet support it however).

In this context I am just using

llvm-project/clang/include/clang/AST/Expr.h

Line 544 in 80737d2

getIntegerConstantExpr(const ASTContext &Ctx,

to define what a constant expression is

That is the tool I'd reach for as well.