[clang] consistently quote expressions in diagnostics #134769
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mizvekov
requested review from
AaronBallman,
Endilll,
cor3ntin,
erichkeane,
shafik and
zyn0217
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Matheus Izvekov (mizvekov) ChangesThis changes the expression diagnostic printer to always add quotes, and removes hardcoded quotes from the diagnostic format strings. In some cases, a placeholder could be filled by either an expression or as a string. In order to quote this consistently, a new modifier was implemented, which can be used to quote strings. One diagnostic kind was relying on unquoted expressions in order to generate code suggestions. This diagnostic is converted to use fixit hints instead. Patch is 89.65 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/134769.diff 51 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e671183522565..2bf74d52da6c6 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -283,6 +283,7 @@ Improvements to Clang's diagnostics
- Clang now respects the current language mode when printing expressions in
diagnostics. This fixes a bunch of `bool` being printed as `_Bool`, and also
a bunch of HLSL types being printed as their C++ equivalents.
+- Clang now consistently quotes expressions in diagnostics.
- When printing types for diagnostics, clang now doesn't suppress the scopes of
template arguments contained within nested names.
- The ``-Wshift-bool`` warning has been added to warn about shifting a boolean. (#GH28334)
diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
index 8d69a2f2cf4a3..f73963752bb67 100644
--- a/clang/include/clang/Basic/DiagnosticASTKinds.td
+++ b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -143,8 +143,8 @@ def note_constexpr_null_subobject : Note<
"access array element of|perform pointer arithmetic on|"
"access real component of|"
"access imaginary component of}0 null pointer">;
-def note_constexpr_null_callee : Note<
- "'%0' evaluates to a null function pointer">;
+def note_constexpr_null_callee
+ : Note<"%0 evaluates to a null function pointer">;
def note_constexpr_function_param_value_unknown : Note<
"function parameter %0 with unknown value cannot be used in a constant "
"expression">;
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index f46e7fed28794..7a3cac528a363 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1639,7 +1639,7 @@ def note_omp_ctx_compatible_set_and_selector_for_property
"'match(%2={%1(%0)})'">;
def warn_omp_ctx_incompatible_score_for_property
: Warning<"the context selector '%0' in the context set '%1' cannot have a "
- "score ('%2'); score ignored">,
+ "score (%quoted2); score ignored">,
InGroup<OpenMPClauses>;
def warn_omp_more_one_device_type_clause
: Warning<"more than one 'device_type' clause is specified">,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 1ad09aba60935..5b35dc353b968 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -167,9 +167,10 @@ def err_expr_not_string_literal : Error<"expression is not a string literal">;
def ext_predef_outside_function : Warning<
"predefined identifier is only valid inside function">,
InGroup<DiagGroup<"predefined-identifier-outside-function">>;
-def ext_init_from_predefined : ExtWarn<
- "initializing an array from a '%0' predefined identifier is a Microsoft extension">,
- InGroup<MicrosoftInitFromPredefined>;
+def ext_init_from_predefined
+ : ExtWarn<"initializing an array from a %0 predefined identifier is a "
+ "Microsoft extension">,
+ InGroup<MicrosoftInitFromPredefined>;
def ext_string_literal_from_predefined : ExtWarn<
"expansion of predefined identifier '%0' to a string literal is a Microsoft extension">,
InGroup<MicrosoftStringLiteralFromPredefined>;
@@ -3019,14 +3020,14 @@ def note_substituted_constraint_expr_is_ill_formed : Note<
"because substituted constraint expression is ill-formed%0">;
def note_constraint_references_error
: Note<"constraint depends on a previously diagnosed expression">;
-def note_atomic_constraint_evaluated_to_false : Note<
- "%select{and|because}0 '%1' evaluated to false">;
-def note_concept_specialization_constraint_evaluated_to_false : Note<
- "%select{and|because}0 '%1' evaluated to false">;
+def note_atomic_constraint_evaluated_to_false
+ : Note<"%select{and|because}0 %1 evaluated to false">;
+def note_concept_specialization_constraint_evaluated_to_false
+ : Note<"%select{and|because}0 %1 evaluated to false">;
def note_single_arg_concept_specialization_constraint_evaluated_to_false : Note<
"%select{and|because}0 %1 does not satisfy %2">;
-def note_atomic_constraint_evaluated_to_false_elaborated : Note<
- "%select{and|because}0 '%1' (%2 %3 %4) evaluated to false">;
+def note_atomic_constraint_evaluated_to_false_elaborated
+ : Note<"%select{and|because}0 %1 (%2 %3 %4) evaluated to false">;
def note_is_deducible_constraint_evaluated_to_false : Note<
"cannot deduce template arguments for %0 from %1">;
def err_constrained_virtual_method : Error<
@@ -3046,14 +3047,14 @@ def note_expr_requirement_expr_substitution_error : Note<
"%select{and|because}0 '%1' would be invalid: %2">;
def note_expr_requirement_expr_unknown_substitution_error : Note<
"%select{and|because}0 '%1' would be invalid">;
-def note_expr_requirement_noexcept_not_met : Note<
- "%select{and|because}0 '%1' may throw an exception">;
+def note_expr_requirement_noexcept_not_met
+ : Note<"%select{and|because}0 %1 may throw an exception">;
def note_expr_requirement_type_requirement_substitution_error : Note<
"%select{and|because}0 '%1' would be invalid: %2">;
def note_expr_requirement_type_requirement_unknown_substitution_error : Note<
"%select{and|because}0 '%1' would be invalid">;
-def note_expr_requirement_constraints_not_satisfied : Note<
- "%select{and|because}0 type constraint '%1' was not satisfied:">;
+def note_expr_requirement_constraints_not_satisfied
+ : Note<"%select{and|because}0 type constraint %1 was not satisfied:">;
def note_expr_requirement_constraints_not_satisfied_simple : Note<
"%select{and|because}0 %1 does not satisfy %2:">;
def note_type_requirement_substitution_error : Note<
@@ -4352,9 +4353,9 @@ def note_reference_is_return_value : Note<"%0 returns a reference">;
def note_pointer_declared_here : Note<
"pointer %0 declared here">;
-def warn_division_sizeof_ptr : Warning<
- "'%0' will return the size of the pointer, not the array itself">,
- InGroup<DiagGroup<"sizeof-pointer-div">>;
+def warn_division_sizeof_ptr
+ : Warning<"%0 will return the size of the pointer, not the array itself">,
+ InGroup<DiagGroup<"sizeof-pointer-div">>;
def warn_division_sizeof_array : Warning<
"expression does not compute the number of elements in this array; element "
"type is %0, not %1">,
@@ -5577,8 +5578,9 @@ def note_function_member_spec_matched : Note<
def err_template_recursion_depth_exceeded : Error<
"recursive template instantiation exceeded maximum depth of %0">,
DefaultFatal, NoSFINAE;
-def err_constraint_depends_on_self : Error<
- "satisfaction of constraint '%0' depends on itself">, NoSFINAE;
+def err_constraint_depends_on_self
+ : Error<"satisfaction of constraint %0 depends on itself">,
+ NoSFINAE;
def note_template_recursion_depth : Note<
"use -ftemplate-depth=N to increase recursive template instantiation depth">;
@@ -7094,8 +7096,8 @@ def warn_precedence_bitwise_rel : Warning<
InGroup<Parentheses>;
def note_precedence_bitwise_first : Note<
"place parentheses around the %0 expression to evaluate it first">;
-def note_precedence_silence : Note<
- "place parentheses around the '%0' expression to silence this warning">;
+def note_precedence_silence : Note<"place parentheses around the %quoted0 "
+ "expression to silence this warning">;
def warn_precedence_conditional : Warning<
"operator '?:' has lower precedence than '%0'; '%0' will be evaluated first">,
@@ -7113,9 +7115,11 @@ def warn_consecutive_comparison : Warning<
def warn_enum_constant_in_bool_context : Warning<
"converting the enum constant to a boolean">,
InGroup<IntInBoolContext>, DefaultIgnore;
-def warn_left_shift_in_bool_context : Warning<
- "converting the result of '<<' to a boolean; did you mean '(%0) != 0'?">,
- InGroup<IntInBoolContext>, DefaultIgnore;
+def warn_left_shift_in_bool_context
+ : Warning<"converting the result of '<<' to a boolean; did you mean to "
+ "compare with '0'?">,
+ InGroup<IntInBoolContext>,
+ DefaultIgnore;
def warn_logical_instead_of_bitwise : Warning<
"use of logical '%0' with constant operand">,
InGroup<DiagGroup<"constant-logical-operand">>;
@@ -10308,9 +10312,11 @@ def warn_dangling_pointer_assignment : Warning<
"object backing %select{|the pointer }0%1 "
"will be destroyed at the end of the full-expression">,
InGroup<DanglingAssignment>;
-def warn_dangling_reference_captured : Warning<
- "object whose reference is captured by '%0' will be destroyed at the end of "
- "the full-expression">, InGroup<DanglingCapture>;
+def warn_dangling_reference_captured
+ : Warning<"object whose reference is captured by %0 will be destroyed at "
+ "the end of "
+ "the full-expression">,
+ InGroup<DanglingCapture>;
def warn_dangling_reference_captured_by_unknown : Warning<
"object whose reference is captured will be destroyed at the end of "
"the full-expression">, InGroup<DanglingCapture>;
@@ -13009,15 +13015,15 @@ def note_acc_atomic_too_many_stmts
def note_acc_atomic_expected_binop : Note<"expected binary operation on right "
"hand side of assignment operator">;
def note_acc_atomic_mismatch_operand
- : Note<"left hand side of assignment operation('%0') must match one side "
- "of the sub-operation on the right hand side('%1' and '%2')">;
+ : Note<"left hand side of assignment operation(%0) must match one side "
+ "of the sub-operation on the right hand side(%1 and %2)">;
def note_acc_atomic_mismatch_compound_operand
: Note<"variable %select{|in unary expression|on right hand side of "
"assignment|on left hand side of assignment|on left hand side of "
- "compound assignment|on left hand side of assignment}2('%3') must "
+ "compound assignment|on left hand side of assignment}2(%3) must "
"match variable used %select{|in unary expression|on right hand "
"side of assignment|<not possible>|on left hand side of compound "
- "assignment|on left hand side of assignment}0('%1') from the first "
+ "assignment|on left hand side of assignment}0(%1) from the first "
"statement">;
def err_acc_declare_required_clauses
: Error<"no valid clauses specified in OpenACC 'declare' directive">;
@@ -13037,9 +13043,9 @@ def err_acc_multiple_references
: Error<"variable referenced in '%0' clause of OpenACC 'declare' directive "
"was already referenced">;
def err_acc_routine_not_func
- : Error<"OpenACC routine name '%0' does not name a function">;
+ : Error<"OpenACC routine name %0 does not name a function">;
def err_acc_routine_overload_set
- : Error<"OpenACC routine name '%0' names a set of overloads">;
+ : Error<"OpenACC routine name %0 names a set of overloads">;
def err_acc_magic_static_in_routine
: Error<"function static variables are not permitted in functions to which "
"an OpenACC 'routine' directive applies">;
diff --git a/clang/lib/AST/ASTDiagnostic.cpp b/clang/lib/AST/ASTDiagnostic.cpp
index ccfef9c7ae361..0c3b30bd264e0 100644
--- a/clang/lib/AST/ASTDiagnostic.cpp
+++ b/clang/lib/AST/ASTDiagnostic.cpp
@@ -512,8 +512,6 @@ void clang::FormatASTNodeDiagnosticArgument(
const Expr *E = reinterpret_cast<Expr *>(Val);
assert(E && "Received null Expr!");
E->printPretty(OS, /*Helper=*/nullptr, Context.getPrintingPolicy());
- // FIXME: Include quotes when printing expressions.
- NeedQuotes = false;
break;
}
}
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index 4b4a85aaccf8b..538c1d18a8ac1 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -1144,20 +1144,25 @@ FormatDiagnostic(const char *DiagStr, const char *DiagEnd,
switch (Kind) {
// ---- STRINGS ----
- case DiagnosticsEngine::ak_std_string: {
- const std::string &S = getArgStdStr(ArgNo);
- assert(ModifierLen == 0 && "No modifiers for strings yet");
- EscapeStringForDiagnostic(S, OutStr);
- break;
- }
+ case DiagnosticsEngine::ak_std_string:
case DiagnosticsEngine::ak_c_string: {
- const char *S = getArgCStr(ArgNo);
- assert(ModifierLen == 0 && "No modifiers for strings yet");
-
- // Don't crash if get passed a null pointer by accident.
- if (!S)
- S = "(null)";
+ StringRef S = [&]() -> StringRef {
+ if (Kind == DiagnosticsEngine::ak_std_string)
+ return getArgStdStr(ArgNo);
+ const char *SZ = getArgCStr(ArgNo);
+ // Don't crash if get passed a null pointer by accident.
+ return SZ ? SZ : "(null)";
+ }();
+ bool Quoted = false;
+ if (ModifierIs(Modifier, ModifierLen, "quoted")) {
+ Quoted = true;
+ OutStr.push_back('\'');
+ } else {
+ assert(ModifierLen == 0 && "unknown modifier for string");
+ }
EscapeStringForDiagnostic(S, OutStr);
+ if (Quoted)
+ OutStr.push_back('\'');
break;
}
// ---- INTEGERS ----
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index bffd0dd461d3d..204c8d94573e7 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -11683,7 +11683,10 @@ static void DiagnoseIntInBoolContext(Sema &S, Expr *E) {
S.Diag(ExprLoc, diag::warn_left_shift_always)
<< (Result.Val.getInt() != 0);
else if (E->getType()->isSignedIntegerType())
- S.Diag(ExprLoc, diag::warn_left_shift_in_bool_context) << E;
+ S.Diag(ExprLoc, diag::warn_left_shift_in_bool_context)
+ << FixItHint::CreateInsertion(E->getBeginLoc(), "(")
+ << FixItHint::CreateInsertion(S.getLocForEndOfToken(E->getEndLoc()),
+ ") != 0");
}
}
diff --git a/clang/test/AST/ByteCode/literals.cpp b/clang/test/AST/ByteCode/literals.cpp
index 68d400bc31dd7..6b33c5cc22367 100644
--- a/clang/test/AST/ByteCode/literals.cpp
+++ b/clang/test/AST/ByteCode/literals.cpp
@@ -920,7 +920,7 @@ namespace CompoundLiterals {
// null pointer suggests we're doing something odd during constant expression
// evaluation: I think it's still taking 'x' as being null from the call to
// f3() rather than tracking the assignment happening in the VLA.
- constexpr int f3(int *x, int (*y)[*(x=(int[]){1,2,3})]) { // both-warning {{object backing the pointer x will be destroyed at the end of the full-expression}}
+ constexpr int f3(int *x, int (*y)[*(x=(int[]){1,2,3})]) { // both-warning {{object backing the pointer 'x' will be destroyed at the end of the full-expression}}
return x[0]; // both-note {{read of dereferenced null pointer is not allowed in a constant expression}}
}
constexpr int h = f3(0,0); // both-error {{constexpr variable 'h' must be initialized by a constant expression}} \
diff --git a/clang/test/CodeGenCXX/sections.cpp b/clang/test/CodeGenCXX/sections.cpp
index c7fe4e45ea05a..88efd7692b1e0 100644
--- a/clang/test/CodeGenCXX/sections.cpp
+++ b/clang/test/CodeGenCXX/sections.cpp
@@ -18,19 +18,19 @@ int D = 1;
#pragma data_seg(".data")
int a = 1;
extern const Mutable mutable_custom_section;
-const Mutable mutable_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'mutable_custom_section' due to the presence of a mutable field}}
+const Mutable mutable_custom_section; // expected-warning {{`#pragma const_seg` for section '".my_const"' will not apply to 'mutable_custom_section' due to the presence of a mutable field}}
extern const Normal normal_custom_section;
const Normal normal_custom_section;
struct NonTrivialDtor {
~NonTrivialDtor();
};
extern const NonTrivialDtor non_trivial_dtor_custom_section;
-const NonTrivialDtor non_trivial_dtor_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'non_trivial_dtor_custom_section' due to the presence of a non-trivial destructor}}
+const NonTrivialDtor non_trivial_dtor_custom_section; // expected-warning {{`#pragma const_seg` for section '".my_const"' will not apply to 'non_trivial_dtor_custom_section' due to the presence of a non-trivial destructor}}
struct NonTrivialCtor {
NonTrivialCtor();
};
extern const NonTrivialCtor non_trivial_ctor_custom_section;
-const NonTrivialCtor non_trivial_ctor_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'non_trivial_ctor_custom_section' due to the presence of a non-trivial constructor}}
+const NonTrivialCtor non_trivial_ctor_custom_section; // expected-warning {{`#pragma const_seg` for section '".my_const"' will not apply to 'non_trivial_ctor_custom_section' due to the presence of a non-trivial constructor}}
#pragma data_seg(push, label, ".data2")
extern const int b;
const int b = 1;
diff --git a/clang/test/FixIt/fixit-bool.cpp b/clang/test/FixIt/fixit-bool.cpp
new file mode 100644
index 0000000000000..ddc8f7f7cdd22
--- /dev/null
+++ b/clang/test/FixIt/fixit-bool.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -fdiagnostics-parseable-fixits -std=c++26 -Wint-in-bool-context %s 2>&1 | FileCheck %s
+
+int x;
+bool t1 = x << x;
+// CHECK-LABEL: 4:13: warning: converting the result of '<<' to a boolean
+// CHECK: fix-it:"{{.*}}":{4:11-4:11}:"("
+// CHECK-NEXT: fix-it:"{{.*}}":{4:17-4:17}:") != 0"
diff --git a/clang/test/Modules/odr_hash.cpp b/clang/test/Modules/odr_hash.cpp
index b0f5b904f2b39..da24b1fe8729a 100644
--- a/clang/test/Modules/odr_hash.cpp
+++ b/clang/test/Modules/odr_hash.cpp
@@ -3380,8 +3380,8 @@ struct S17 {
};
#else
S17 s17;
-// [email protected]:* {{'FunctionTemplate::S17' has different definitions in different modules; first difference is definition in module 'SecondModule' found function template 'foo' with 1st template parameter with default argument 1 + 1}}
-// [email protected]:* {{but in 'FirstModule' found function template 'foo' with 1st template parameter with default argument 2}}
+// [email protected]:* {{'FunctionTemplate::S17' has different definitions in different modules; first difference is definition in module 'SecondModule' found function template 'foo' with 1st template parameter with default argument '1 + 1'}}
+// [email protected]:* {{but in 'FirstModule' found function template 'foo' with 1st template parameter with default argument '2'}}
#endif
#if defined(FIRST)
diff --git a/clang/test/OpenMP/declare_variant_messages.c b/clang/test/OpenMP/declare_variant_messages.c
index 14637d8200671..32e365cc415bd 100644
--- a/clang/test/OpenMP/declare_variant_messages.c
+++ b/clang/test/OpenMP/declare_variant_messages.c
@@ -34,7 +34,7 @@ int foo(void);
#pragma omp declare variant(foo) match(implementation={vendor(score ibm)}) // expected-error {{expected '(' after 'score'}} expected-warning {{expected '':'' after the score expression; '':'' assumed}}
#pragma omp declare variant(foo) match(implementation={vendor(score( ibm)}) // expected-error {{use of undeclared identifier 'ibm'}} expected-error {{expected ')'}} expected-warning {{expected '':'' after the score expression; '':'' assumed}} expected-warning {{expected identifier or string literal describing a context property; property skipped}} expected-note {{context property options are: 'amd' 'arm' 'bsc' 'cray' 'fujitsu' 'gnu' 'ibm' 'intel' 'llvm' 'nec' 'nvidia' 'pgi' 'ti' 'unknown'}} expected-note {{to match this '('}}
#pragma omp declare variant(foo) match(implementation={vendor(score(2 ibm)}) // expected-error {{expected ')'}} expected-error {{expected ')'}} expected-warning {{expected '':'' after the score expression; '':'' assumed}} expected-warning {{expected identifier or string literal describing a context property; property skipped}} expected-note {{to match this '('}} expected-note {{context property options are: 'amd' 'arm' 'bsc' 'cray' 'fujitsu' 'gnu' 'ibm' 'intel' 'llvm' 'nec' 'nvidia' 'pgi' 'ti' 'unknown'}} expected-note {{to match this '('}}
-#pragma omp declare variant(foo) match(implementation={vendor(score(foo()) ibm)}) // expected-warning {{expected '':'' after the score expression; '':'' assumed}} expected-warning {{score expressions in the OpenMP context se...
[truncated]
|
zyn0217
reviewed
Apr 8, 2025
Contributor
zyn0217
left a comment
zyn0217
left a comment
There was a problem hiding this comment.
Thanks for working productively.
Comment on lines
+1149
to
+1162
| StringRef S = [&]() -> StringRef { | ||
| if (Kind == DiagnosticsEngine::ak_std_string) | ||
| return getArgStdStr(ArgNo); | ||
| const char *SZ = getArgCStr(ArgNo); | ||
| // Don't crash if get passed a null pointer by accident. | ||
| return SZ ? SZ : "(null)"; | ||
| }(); | ||
| bool Quoted = false; | ||
| if (ModifierIs(Modifier, ModifierLen, "quoted")) { | ||
| Quoted = true; | ||
| OutStr.push_back('\''); | ||
| } else { | ||
| assert(ModifierLen == 0 && "unknown modifier for string"); | ||
| } |
Contributor
There was a problem hiding this comment.
Instead of adding quotes in the DiagnosticEngine, can we teach our tablegen to generate quotes for %quote specifier?
Of course I wouldn't insist if that would end up too much churn to the implementation.
Collaborator
There was a problem hiding this comment.
It would be somewhat novel for us to do string modification of the diagnostic text in tablegen, I think setting it here is sensible. But @AaronBallman probably can take a look here and give an opinion.
Contributor
Author
There was a problem hiding this comment.
Yeah, the modifiers are specified in tablegen so it can validate its input, but the semantics are currently all implemented outside.
Ideally we could always quote the strings here, same as everything else, but the problem is that currently there are a bunch of places which are assembling the diagnostic prose itself from strings, which is something that should not happen, as it makes the job of localization much more difficult.
Contributor
There was a problem hiding this comment.
there are a bunch of places which are assembling the diagnostic prose itself from strings
Like concept diagnostics?
Contributor
Author
There was a problem hiding this comment.
Yes that's one example.
Contributor
There was a problem hiding this comment.
Do we want to assert we don't end up with double quotes?
Contributor
Author
There was a problem hiding this comment.
That's not impossible, but we suffer from the problem generally, and is one reason we should be pointing to things, not printing them.
cor3ntin
reviewed
Apr 8, 2025
Comment on lines
+11686
to
+11691
| S.Diag(ExprLoc, diag::warn_left_shift_in_bool_context) | ||
| << FixItHint::CreateInsertion(E->getBeginLoc(), "(") | ||
| << FixItHint::CreateInsertion(S.getLocForEndOfToken(E->getEndLoc()), | ||
| ") != 0"); |
Contributor
Author
There was a problem hiding this comment.
Explained in the commit message, but this would look incorrect with quotes as it was pasting code together.
Comment on lines
+1149
to
+1162
| StringRef S = [&]() -> StringRef { | ||
| if (Kind == DiagnosticsEngine::ak_std_string) | ||
| return getArgStdStr(ArgNo); | ||
| const char *SZ = getArgCStr(ArgNo); | ||
| // Don't crash if get passed a null pointer by accident. | ||
| return SZ ? SZ : "(null)"; | ||
| }(); | ||
| bool Quoted = false; | ||
| if (ModifierIs(Modifier, ModifierLen, "quoted")) { | ||
| Quoted = true; | ||
| OutStr.push_back('\''); | ||
| } else { | ||
| assert(ModifierLen == 0 && "unknown modifier for string"); | ||
| } |
Contributor
There was a problem hiding this comment.
Do we want to assert we don't end up with double quotes?
AaronBallman
reviewed
Apr 8, 2025
Collaborator
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
Please be sure to update
llvm-project/clang/docs/InternalsManual.rst
Line 227 in 46d4c3b
%quoted specifier is explicitly documented.
mizvekov
force-pushed
the
users/mizvekov/diag-expr-quoted
branch
from
270ce96 to
36a48fa
Compare
zyn0217
reviewed
Apr 9, 2025
Comment on lines
+146
to
+147
| def note_constexpr_null_callee | ||
| : Note<"%0 evaluates to a null function pointer">; |
Contributor
There was a problem hiding this comment.
Just out of curiosity: do you have a formatter running for td files? I hardly remembered clang-format would handle it.
Contributor
Author
There was a problem hiding this comment.
Yeah, clang-format gained support for these files a while back.
| int a = 1; | ||
| extern const Mutable mutable_custom_section; | ||
| const Mutable mutable_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'mutable_custom_section' due to the presence of a mutable field}} | ||
| const Mutable mutable_custom_section; // expected-warning {{`#pragma const_seg` for section '".my_const"' will not apply to 'mutable_custom_section' due to the presence of a mutable field}} |
Contributor
There was a problem hiding this comment.
This looks suspicious. Is '" what we want?
Contributor
Author
There was a problem hiding this comment.
Yeah this is printing the string expression, instead of resolving it to the actual section name.
I think that this was bad to start with, and the fact this now gets quoted doesn't make that much worse.
I was trying to avoid fixing yet another pre-existing diagnostic issue and making this patch more complicated than it strictly needs to be.
Contributor
Author
|
I plan to merge this tomorrow if there are no further comments. |
erichkeane
approved these changes
Apr 14, 2025
Collaborator
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
Woops! Thought I'd already approved this.
mizvekov
force-pushed
the
users/mizvekov/diag-expr-quoted
branch
2 times, most recently
from
7a77796 to
7fe930d
Compare
This changes the expression diagnostic printer to always add quotes, and removes hardcoded quotes from the diagnostic format strings. In some cases, a placeholder could be filled by either an expression or as a string. In order to quote this consistently, a new modifier was added, which can be used to quote strings. One diagnostic was relying on unquoted expressions in order to generate code suggestions. This diagnostic is converted to use fixit hints instead.
mizvekov
force-pushed
the
users/mizvekov/diag-expr-quoted
branch
from
7fe930d to
e2c4004
Compare
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/16037 Here is the relevant piece of the build log for the reference |
Member
|
@mizvekov while with this change we get: Is this something we want? The I'm less interested in changing anyone's stance on this: I'm quite happy with this change overall. I'm more interested in establishing a clear style guide for these cases, so that we can be consistent with the upstream style. |
Contributor
Author
|
Yeah, this is something we want to fix. This problem already existed for the cases we already quoted expressions when printing them. The opposite problem of an expression mixing in with the prose of the diagnostic disappeared though. I think this is still the direction we want to move on to. It seems easier to fix the double quote problem, than the reverse case of making the expression stand out in the prose without them. One solution I have been thinking, is to recognize certain kinds of expressions which would print weirdly because they already have their own delimiters, and printing them with a special case. For example we could say, instead of ''x'', we say "the character literal 'x'". We could do similarly for strings as well. I think this looks better than trying to escape them. |
Member
|
Right, yeah. Thanks for working on this! We have quite a few diagnostics for bounds safety to help guide users, so good diagnostics are important to us. |
hnrklssn
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Apr 23, 2025
In fceb9ce (llvm#134769) upstream changed the diagnostic engine to always wrap expressions in single quotes. We already had manual wrapping for most of those, so the manual single quotes needed to be removed. In other placed the test cases needed to be updated to reflect the new output. rdar://149359009
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes the expression diagnostic printer to always add quotes, and removes hardcoded quotes from the diagnostic format strings.
In some cases, a placeholder could be filled by either an expression or as a string. In order to quote this consistently, a new modifier was implemented, which can be used to quote strings.
One diagnostic kind was relying on unquoted expressions in order to generate code suggestions. This diagnostic is converted to use fixit hints instead.