rewording note note_constexpr_invalid_cast

[Codesbyusman]

Summary

The diagnostics here are correct, but the note is really silly. It talks about reinterpret_cast in C code. So rewording it for c mode by using more select.

int array[(long)(char *)0];

previous note:

cast that performs the conversions of a reinterpret_cast is not allowed in a constant expression

reworded note:

this conversion is not allowed in a constant expression

[aaron.ballman]

Thanks for the cleanup here! Precommit CI pointed out places where test coverage found issues, and you're missing new test coverage for the change. If you run the failing constant-expression-cxx11.cpp test in an asserts build, it'll help you to narrow down where the issue is. I think the problem is that there are more instances of issuing diag::note_constexpr_invalid_cast with the trailing argument 2 that weren't updated to pass the additional trailing argument you added.

[shafik]

I can't find any C tests that exercise this diagnostic. We should add a C test as well to confirm that we obtain the diagnostic we expect in a C context.

[Codesbyusman]

updated the test work for the patch

[Codesbyusman]

Hi,
I am not getting why Sema/cast.c is failing here. While all test are running fine on my system?

[Codesbyusman]

updating one time again, to again run test as no failed test on my system.

[aaron.ballman]

clang/test/Sema/cast.c

2

Instead of adding this RUN line, I think you can modify the previous RUN line to add -Wvla, and skip the #ifdef entirely.

The reason this test is passing for you locally but failing on the server is because this RUN line is missing a triple and there are parts of the test which are sensitive to the target architecture.

[Codesbyusman]

clang/test/Sema/cast.c

2

Instead of adding this RUN line, I think you can modify the previous RUN line to add -Wvla, and skip the #ifdef entirely.

The reason this test is passing for you locally but failing on the server is because this RUN line is missing a triple and there are parts of the test which are sensitive to the target architecture.

ok, Thank you

[Codesbyusman]

updating test

[aaron.ballman]

clang/lib/AST/ExprConstant.cpp

7475–7481

You shouldn't pass the extra argument to either of these -- 0 maps to reinterpret_cast and 1 maps to dynamic_cast in the %select{}, neither of which use a %1 placeholder.

8893

This one should also drop the extra streamed argument for similar reasons as above.

clang/test/Sema/cast.c

1–6

I realized that we don't need to pass -Wvla at all, the other warning and note are on by default.

clang/test/SemaCXX/constant-expression-cxx11.cpp

3

We don't need to use -Wvla to see the diagnostic change, and this will fix the test failures from the other RUN lines not generating the same warning about use of a VLA.

14–16

[Codesbyusman]

clang/test/Sema/cast.c

1–6

I realized that we don't need to pass -Wvla at all, the other warning and note are on by default.

But the note is not enabled without it, without it i got this error:
that I am expecting but there is no diagnostic for it

error: 'note' diagnostics expected but not seen:

File /home/kali/llvm-project/clang/test/Sema/cast.c Line 3: this conversion is not allowed in a constant expression

1 error generated.

[aaron.ballman]

clang/test/Sema/cast.c

1–6

Oh! I was thinking that the note was associated with the constant folding warning, not the "you used a VLA" warning, but you're absolutely right! Ignore the suggestion and keep passing -Wvla here.

clang/test/SemaCXX/constant-expression-cxx11.cpp

3

And this is a better suggestion here.

14–16

[Codesbyusman]

clang/test/SemaCXX/constant-expression-cxx11.cpp

3

Sorry I forget to mention removing here in cpp case -Wvla was working so go with vla or without it?

[aaron.ballman]

clang/test/SemaCXX/constant-expression-cxx11.cpp

3

Let's go with the vla here as well.

[Codesbyusman]

updated, I am not getting why this is happening but in the .cpp test file without Wvla all is working and is detecting by default as you are saying but when I use vla-warning/note it gives me error that "warning seen but not expected"

[aaron.ballman]

In D133194#3781271, @Codesbyusman wrote:

updated, I am not getting why this is happening but in the .cpp test file without Wvla all is working and is detecting by default as you are saying but when I use vla-warning/note it gives me error that "warning seen but not expected"

It's because we're passing -pedantic on all three RUN lines; because the file is a C++ source file, we issue the pedantic "you're using an extension" warning: https://godbolt.org/z/a8nv6dhE5 so the -Wvla is redundant after all (sorry for not catching that sooner).

clang/lib/AST/ExprConstant.cpp

7475–7476

[Codesbyusman]

In D133194#3782198, @aaron.ballman wrote:

In D133194#3781271, @Codesbyusman wrote:

updated, I am not getting why this is happening but in the .cpp test file without Wvla all is working and is detecting by default as you are saying but when I use vla-warning/note it gives me error that "warning seen but not expected"

It's because we're passing -pedantic on all three RUN lines; because the file is a C++ source file, we issue the pedantic "you're using an extension" warning: https://godbolt.org/z/a8nv6dhE5 so the -Wvla is redundant after all (sorry for not catching that sooner).

Thank you for explanation.

[Codesbyusman]

updating with the suggested changes.

clang/docs/ReleaseNotes.rst

 - Correctly diagnose a future keyword if it exist as a keyword in the higher
   language version and specifies in which version it will be a keyword. This
   supports both c and c++ language.
 - When diagnosing multi-level pack expansions of mismatched lengths, Clang will
   now, in most cases, be able to point to the relevant outer parameter.
 - no_sanitize("...") on a global variable for known but not relevant sanitizers
   is now just a warning. It now says that this will be ignored instead of
   incorrectly saying no_sanitize only applies to functions and methods.
+- No longer mention ``reinterpet_cast`` in the invalid constant expression
+  diagnostic note when in C mode.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 - It's now possible to set the crash diagnostics directory through
   the environment variable ``CLANG_CRASH_DIAGNOSTICS_DIR``.
   The ``-fcrash-diagnostics-dir`` flag takes precedence.
 - When using header modules, inclusion of a private header and violations of
   the `use-declaration rules

clang/include/clang/Basic/DiagnosticASTKinds.td

 //==--- DiagnosticASTKinds.td - libast diagnostics ------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 let Component = "AST" in {
 
 // Constant expression diagnostics. These (and their users) belong in Sema.
 def note_expr_divide_by_zero : Note<"division by zero">;
 def note_constexpr_invalid_cast : Note<
-  "%select{reinterpret_cast|dynamic_cast|cast that performs the conversions of"
-  " a reinterpret_cast|cast from %1}0 is not allowed in a constant expression"
+  "%select{reinterpret_cast|dynamic_cast|%select{this conversion|cast that"
+  " performs the conversions of a reinterpret_cast}1|cast from %1}0"
+  " is not allowed in a constant expression"
   "%select{| in C++ standards before C++20||}0">;
 def note_constexpr_invalid_downcast : Note<
   "cannot cast object of dynamic type %0 to type %1">;
 def note_constexpr_overflow : Note<
   "value %0 is outside the range of representable values of type %1">;
 def note_constexpr_negative_shift : Note<"negative shift count %0">;
 def note_constexpr_large_shift : Note<
   "shift count %0 >= width of type %1 (%2 bit%s2)">;

clang/lib/AST/ExprConstant.cpp

public:
 
   // Temporaries are registered when created, so we don't care about
   // CXXBindTemporaryExpr.
   bool VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *E) {
     return StmtVisitorTy::Visit(E->getSubExpr());
   }
 
   bool VisitCXXReinterpretCastExpr(const CXXReinterpretCastExpr *E) {
     CCEDiag(E, diag::note_constexpr_invalid_cast) << 0;
     return static_cast<Derived*>(this)->VisitCastExpr(E);
   }
   bool VisitCXXDynamicCastExpr(const CXXDynamicCastExpr *E) {
     if (!Info.Ctx.getLangOpts().CPlusPlus20)
       CCEDiag(E, diag::note_constexpr_invalid_cast) << 1;
     return static_cast<Derived*>(this)->VisitCastExpr(E);
   }
   bool VisitBuiltinBitCastExpr(const BuiltinBitCastExpr *E) {
     return static_cast<Derived*>(this)->VisitCastExpr(E);
   }
 
   bool VisitBinaryOperator(const BinaryOperator *E) {
     switch (E->getOpcode()) {
     default:
public:
   bool VisitCompoundAssignOperator(const CompoundAssignOperator *CAO);
 
   bool VisitCastExpr(const CastExpr *E) {
     switch (E->getCastKind()) {
     default:
       return LValueExprEvaluatorBaseTy::VisitCastExpr(E);
 
     case CK_LValueBitCast:
-      this->CCEDiag(E, diag::note_constexpr_invalid_cast) << 2;
+      this->CCEDiag(E, diag::note_constexpr_invalid_cast)
+          << 2 << Info.Ctx.getLangOpts().CPlusPlus;
       if (!Visit(E->getSubExpr()))
         return false;
       Result.Designator.setInvalid();
       return true;
 
     case CK_BaseToDerived:
       if (!Visit(E->getSubExpr()))
         return false;
if (!E->getType()->isVoidPointerType()) {
       if (VoidPtrCastMaybeOK &&
           (Info.getStdAllocatorCaller("allocate") ||
            IsDeclSourceLocationCurrent(Info.CurrentCall->Callee))) {
         // Permitted.
       } else {
         Result.Designator.setInvalid();
         if (SubExpr->getType()->isVoidPointerType())
           CCEDiag(E, diag::note_constexpr_invalid_cast)
               << 3 << SubExpr->getType();
         else
-          CCEDiag(E, diag::note_constexpr_invalid_cast) << 2;
+          CCEDiag(E, diag::note_constexpr_invalid_cast)
+              << 2 << Info.Ctx.getLangOpts().CPlusPlus;
       }
     }
     if (E->getCastKind() == CK_AddressSpaceConversion && Result.IsNullPtr)
       ZeroInitialization(E);
     return true;
 
   case CK_DerivedToBase:
   case CK_UncheckedDerivedToBase:
if (!Visit(E->getSubExpr()))
       return false;
     return HandleDynamicCast(Info, cast<ExplicitCastExpr>(E), Result);
 
   case CK_NullToPointer:
     VisitIgnoredValue(E->getSubExpr());
     return ZeroInitialization(E);
 
   case CK_IntegralToPointer: {
-    CCEDiag(E, diag::note_constexpr_invalid_cast) << 2;
+    CCEDiag(E, diag::note_constexpr_invalid_cast)
+        << 2 << Info.Ctx.getLangOpts().CPlusPlus;
 
     APValue Value;
     if (!EvaluateIntegerOrLValue(SubExpr, Value, Info))
       break;
 
     if (Value.isInt()) {
       unsigned Size = Info.Ctx.getTypeSize(E->getType());
       uint64_t N = Value.getInt().extOrTrunc(Size).getZExtValue();
if (Info.Ctx.getLangOpts().CPlusPlus && Info.InConstantContext &&
       }
     }
 
     return Success(HandleIntToIntCast(Info, E, DestType, SrcType,
                                       Result.getInt()), E);
   }
 
   case CK_PointerToIntegral: {
-    CCEDiag(E, diag::note_constexpr_invalid_cast) << 2;
+    CCEDiag(E, diag::note_constexpr_invalid_cast)
+        << 2 << Info.Ctx.getLangOpts().CPlusPlus;
 
     LValue LV;
     if (!EvaluatePointer(SubExpr, LV, Info))
       return false;
 
     if (LV.getLValueBase()) {
       // Only allow based lvalue casts if they are lossless.
       // FIXME: Allow a larger integer size than the pointer size, and allow

clang/test/Sema/cast.c

-// RUN: %clang_cc1 -fsyntax-only -triple x86_64-unknown-unknown %s -verify
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-unknown-unknown %s -verify -Wvla
+
+int array[(long)(char *)0]; // expected-warning {{variable length array used}} \
+                            // expected-warning {{variable length array folded to constant array as an extension}} \
+                            // expected-note {{this conversion is not allowed in a constant expression}}
 
 typedef struct { unsigned long bits[(((1) + (64) - 1) / (64))]; } cpumask_t;
 cpumask_t x;
 void foo(void) {
   (void)x;
 }
 void bar(void) {
   char* a;
   double b;

clang/test/SemaCXX/constant-expression-cxx11.cpp

 // RUN: %clang_cc1 -std=c++2b -fsyntax-only -verify=expected,cxx20_2b,cxx2b    -triple x86_64-linux -Wno-string-plus-int -Wno-pointer-arith -Wno-zero-length-array -Wno-c99-designator -fcxx-exceptions -pedantic %s -Wno-comment -Wno-tautological-pointer-compare -Wno-bool-conversion
 // RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,cxx11_20,cxx20_2b -triple x86_64-linux -Wno-string-plus-int -Wno-pointer-arith -Wno-zero-length-array -Wno-c99-designator -fcxx-exceptions -pedantic %s -Wno-comment -Wno-tautological-pointer-compare -Wno-bool-conversion
 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify=expected,cxx11_20,cxx11    -triple x86_64-linux -Wno-string-plus-int -Wno-pointer-arith -Wno-zero-length-array -Wno-c99-designator -fcxx-exceptions -pedantic %s -Wno-comment -Wno-tautological-pointer-compare -Wno-bool-conversion
 
 namespace StaticAssertFoldTest {
 
 int x;
 static_assert(++x, "test"); // expected-error {{not an integral constant expression}}
 // cxx20_2b-note@-1 {{cannot modify an object that is visible outside that expression}}
 static_assert(false, "test"); // expected-error {{test}}
 
 }
 
+int array[(long)(char *)0]; // expected-warning {{variable length arrays are a C99 feature}} \
+                            // expected-warning {{variable length array folded to constant array as an extension}} \
+                            // expected-note {{cast that performs the conversions of a reinterpret_cast is not allowed in a constant expression}}
+
 typedef decltype(sizeof(char)) size_t;
 
 template<typename T> constexpr T id(const T &t) { return t; }
 template<typename T> constexpr T min(const T &a, const T &b) {
   return a < b ? a : b;
 }
 template<typename T> constexpr T max(const T &a, const T &b) {
   return a < b ? b : a;