cppcoreguidelines-pro-bounds-constant-array-index: crash for value dependent index in c++03 mode

[mgehre]

Summary

When the expression is value dependent,
isIntegerConstantExpr() crashes in C++03 mode with
../tools/clang/lib/AST/ExprConstant.cpp:9330: (anonymous namespace)::ICEDiag CheckICE(const clang::Expr *, const clang::ASTContext &):

Assertion `!E->isValueDependent() && "Should not see value dependent exprs!"' failed.

In C++11 mode, that assert does not trigger.

This commit works around this in the check, but maybe this
should be fixed in isIntegerConstantExpr?

[aaron.ballman]

Hmm, perhaps @rsmith has other opinions, but I think this should be fixed in isIntegerConstantExpr(). C++03 has the notion of value dependence, and [temp.dep.type]p1 and [expr.const]p1 both specify that a non-type template parameters of integral types are constant expressions.

[mgehre]

Even if we change isIntegerConstantExpr() to return true instead of asserting, we still need this fix to the check. Because
we call isIntegerConstantExpr() to find out if we can possibly calculate the (constant) value of the index expression.
If it is value dependent, we cannot.

[aaron.ballman]

In D22190#479921, @mgehre wrote:

Even if we change isIntegerConstantExpr() to return true instead of asserting, we still need this fix to the check. Because
we call isIntegerConstantExpr() to find out if we can possibly calculate the (constant) value of the index expression.
If it is value dependent, we cannot.

You can on instantiation, can you not?

[mgehre]

Suppress diagnostics if index is value-dependent. Emit them in specializations.

[aaron.ballman]

LGTM!

clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp

70 ↗

(On Diff #63795)

Comments should be complete sentences (capitalization and punctuation).

test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index-c++03.cpp

7 ↗

(On Diff #63795)

Missing a full stop.

[rsmith]

It's not meaningful to ask whether a value-dependent expression is an integral constant expression. The answer is not knowable, so the function asserting doesn't seem unreasonable. Any value we returned from that function would be a lie.

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp

Finder->addMatcher(
           .bind("expr"),
       this);
 }
 
 void ProBoundsConstantArrayIndexCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto *Matched = Result.Nodes.getNodeAs<Expr>("expr");
   const auto *IndexExpr = Result.Nodes.getNodeAs<Expr>("index");
+
+  if (IndexExpr->isValueDependent())
+    return; // We check in the specialization.
+
   llvm::APSInt Index;
   if (!IndexExpr->isIntegerConstantExpr(Index, *Result.Context, nullptr,
                                         /*isEvaluated=*/true)) {
     SourceRange BaseRange;
     if (const auto *ArraySubscriptE = dyn_cast<ArraySubscriptExpr>(Matched))
       BaseRange = ArraySubscriptE->getBase()->getSourceRange();
     else
       BaseRange =

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index-c++03.cpp

+// RUN: clang-tidy %s -checks=-*,cppcoreguidelines-pro-bounds-constant-array-index -- -std=c++03 | count 0
+
+// Note: this test expects no diagnostics, but FileCheck cannot handle that,
+// hence the use of | count 0.
+template <int index> struct B {
+  int get() {
+    // The next line used to crash the check (in C++03 mode only).
+    return x[index];
+  }
+  int x[3];
+};