[clang] Format bitfield width diagnostics with thousands-separators

[tbaederr]

Wondering if we should have a toString() version that does this automatically, or maybe the operator<< for diagnostics for AP(S)Int could even do this? Wouldn't be opt-in anymore though.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Wondering if we should have a toString() version that does this automatically, or maybe the operator<< for diagnostics for AP(S)Int could even do this? Wouldn't be opt-in anymore though.

---

Full diff: https://github.com/llvm/llvm-project/pull/117763.diff

2 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+1-1)
• (modified) clang/lib/Sema/SemaDecl.cpp (+21-6)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 6ff24c2bc8faad..5ab0885c8414fd 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6400,7 +6400,7 @@ def err_incorrect_number_of_vector_initializers : Error<
 // Used by C++ which allows bit-fields that are wider than the type.
 def warn_bitfield_width_exceeds_type_width: Warning<
   "width of bit-field %0 (%1 bits) exceeds the width of its type; value will "
-  "be truncated to %2 bit%s2">, InGroup<BitFieldWidth>;
+  "be truncated to %2 bits">, InGroup<BitFieldWidth>;
 def err_bitfield_too_wide : Error<
   "%select{bit-field %1|anonymous bit-field}0 is too wide (%2 bits)">;
 def warn_bitfield_too_small_for_enum : Warning<
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 74b0e5ad23bd48..7378edc1c5cecb 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -18307,16 +18307,22 @@ ExprResult Sema::VerifyBitField(SourceLocation FieldLoc,
   if (Value.isSigned() && Value.isNegative()) {
     if (FieldName)
       return Diag(FieldLoc, diag::err_bitfield_has_negative_width)
-               << FieldName << toString(Value, 10);
+             << FieldName
+             << toString(Value, 10, Value.isSigned(),
+                         /*formatAsCLiteral=*/false, /*UpperCase=*/true,
+                         /*InsertSeparators=*/true);
     return Diag(FieldLoc, diag::err_anon_bitfield_has_negative_width)
-      << toString(Value, 10);
+           << toString(Value, 10, Value.isSigned(), /*formatAsCLiteral=*/false,
+                       /*UpperCase=*/true, /*InsertSeparators=*/true);
   }
 
   // The size of the bit-field must not exceed our maximum permitted object
   // size.
   if (Value.getActiveBits() > ConstantArrayType::getMaxSizeBits(Context)) {
     return Diag(FieldLoc, diag::err_bitfield_too_wide)
-           << !FieldName << FieldName << toString(Value, 10);
+           << !FieldName << FieldName
+           << toString(Value, 10, Value.isSigned(), /*formatAsCLiteral=*/false,
+                       /*UpperCase=*/true, /*InsertSeparators=*/true);
   }
 
   if (!FieldTy->isDependentType()) {
@@ -18335,7 +18341,10 @@ ExprResult Sema::VerifyBitField(SourceLocation FieldLoc,
       unsigned DiagWidth =
           CStdConstraintViolation ? TypeWidth : TypeStorageSize;
       return Diag(FieldLoc, diag::err_bitfield_width_exceeds_type_width)
-             << (bool)FieldName << FieldName << toString(Value, 10)
+             << (bool)FieldName << FieldName
+             << toString(Value, 10, Value.isSigned(),
+                         /*formatAsCLiteral=*/false, /*UpperCase=*/true,
+                         /*InsertSeparators=*/true)
              << !CStdConstraintViolation << DiagWidth;
     }
 
@@ -18343,9 +18352,15 @@ ExprResult Sema::VerifyBitField(SourceLocation FieldLoc,
     // specified bits as value bits: that's all integral types other than
     // 'bool'.
     if (BitfieldIsOverwide && !FieldTy->isBooleanType() && FieldName) {
+      llvm::APInt TypeWidthAP(sizeof(TypeWidth) * 8, TypeWidth,
+                              /*IsSigned=*/false);
       Diag(FieldLoc, diag::warn_bitfield_width_exceeds_type_width)
-          << FieldName << toString(Value, 10)
-          << (unsigned)TypeWidth;
+          << FieldName
+          << toString(Value, 10, Value.isSigned(), /*formatAsCLiteral=*/false,
+                      /*UpperCase=*/true, /*InsertSeparators=*/true)
+          << toString(TypeWidthAP, 10, /*Signed=*/false,
+                      /*formatAsCLiteral=*/false, /*UpperCase=*/true,
+                      /*InsertSeparators=*/true);
     }
   }
 

[cor3ntin]

Wondering if we should have a toString() version that does this automatically,

Yes, I think either a toStringWithDigitSeparators function, or an overload taking an enum rather than a whole bunch of bool would improve this code a lot.

[AaronBallman]

Wondering if we should have a toString() version that does this automatically,

Yes, I think either a toStringWithDigitSeparators function, or an overload taking an enum rather than a whole bunch of bool would improve this code a lot.

Should we come up with a new format specifier for the diagnostics engine so that the author of the diagnostic can opt in to the additional formatting? My concern with doing this in general is that some numbers really don't need/want the separators so it seems like an opt-in would be better and if we make the opt-in via the diagnostic string itself, nobody has to do anything special to emit the diagnostic.

[Sirraide]

Should we come up with a new format specifier for the diagnostics engine so that the author of the diagnostic can opt in to the additional formatting? My concern with doing this in general is that some numbers really don't need/want the separators so it seems like an opt-in would be better and if we make the opt-in via the diagnostic string itself, nobody has to do anything special to emit the diagnostic.

+1 for this approach.

[tbaederr]

Added a %format format specifier although the name isn't too good.

[Sirraide]

I’m bad at naming, but maybe %sep would work; or maybe %separated or %separators, but those are a bit long.

[tbaederr]

Is the lifetime for the APSInt here OK? When I call Diag(), will the be diagnostic emitted immediately? Or does the APSInt need to life longer?

[Sirraide]

Is the lifetime for the APSInt here OK? When I call Diag(), will the be diagnostic emitted immediately? Or does the APSInt need to life longer?

Iirc that cause problems if e.g. PDiag is used since the diagnostic is stored and emitted later.

[AaronBallman]

Is the lifetime for the APSInt here OK? When I call Diag(), will the be diagnostic emitted immediately? Or does the APSInt need to life longer?

Iirc that cause problems if e.g. PDiag is used since the diagnostic is stored and emitted later.

Yup! Also, we want the ability to batch diagnostics and emit them when it makes the most sense, so I think we want the lifetime of the APSInt to be extended to that of the AST context (then it behaves the same as NamedDecl *).