[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
courbet added a comment. I agree. I'll have a look at it. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55270/new/ https://reviews.llvm.org/D55270 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
Quuxplusone added a comment. @courbet: On the cpplang Slack, Peter Feichtinger mentioned that defaulted template arguments should perhaps be treated differently. Is there any way to suppress the `, std::allocator` part of this diagnostic? https://godbolt.org/z/TM0UHc Before your patches: :11:5: error: static_assert failed due to requirement 'std::is_integral_v' static_assert(std::is_integral_v); ^ ~ After your patches: :11:5: error: static_assert failed due to requirement 'std::is_integral_v > >' static_assert(std::is_integral_v); ^ ~ I don't think the new behavior is //worse//; but I don't think it's optimal, either. I think Clang already has a feature to suppress printing these parameters; you would just have to figure out where it is and try to hook it into your thing. (And if you do, I'm going to ask for a test case where `T::t` is `std::pmr::vector`!) Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55270/new/ https://reviews.llvm.org/D55270 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL348741: [Sema] Further improvements to to static_assert
diagnostics. (authored by courbet, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55270/new/
https://reviews.llvm.org/D55270
Files:
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/lib/Sema/SemaTemplate.cpp
cfe/trunk/test/PCH/cxx-static_assert.cpp
cfe/trunk/test/Sema/static-assert.c
cfe/trunk/test/SemaCXX/static-assert-cxx17.cpp
cfe/trunk/test/SemaCXX/static-assert.cpp
Index: cfe/trunk/lib/Sema/SemaTemplate.cpp
===
--- cfe/trunk/lib/Sema/SemaTemplate.cpp
+++ cfe/trunk/lib/Sema/SemaTemplate.cpp
@@ -3052,30 +3052,42 @@
return Cond;
}
-// Print a diagnostic for the failing static_assert expression. Defaults to
-// pretty-printing the expression.
-static void prettyPrintFailedBooleanCondition(llvm::raw_string_ostream &OS,
- const Expr *FailedCond,
- const PrintingPolicy &Policy) {
- const auto *DR = dyn_cast(FailedCond);
- if (DR && DR->getQualifier()) {
-// If this is a qualified name, expand the template arguments in nested
-// qualifiers.
-DR->getQualifier()->print(OS, Policy, true);
-// Then print the decl itself.
-const ValueDecl *VD = DR->getDecl();
-OS << VD->getName();
-if (const auto *IV = dyn_cast(VD)) {
- // This is a template variable, print the expanded template arguments.
- printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
+namespace {
+
+// A PrinterHelper that prints more helpful diagnostics for some sub-expressions
+// within failing boolean expression, such as substituting template parameters
+// for actual types.
+class FailedBooleanConditionPrinterHelper : public PrinterHelper {
+public:
+ explicit FailedBooleanConditionPrinterHelper(const PrintingPolicy &Policy)
+ : Policy(Policy) {}
+
+ bool handledStmt(Stmt *E, raw_ostream &OS) override {
+const auto *DR = dyn_cast(E);
+if (DR && DR->getQualifier()) {
+ // If this is a qualified name, expand the template arguments in nested
+ // qualifiers.
+ DR->getQualifier()->print(OS, Policy, true);
+ // Then print the decl itself.
+ const ValueDecl *VD = DR->getDecl();
+ OS << VD->getName();
+ if (const auto *IV = dyn_cast(VD)) {
+// This is a template variable, print the expanded template arguments.
+printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
+ }
+ return true;
}
-return;
+return false;
}
- FailedCond->printPretty(OS, nullptr, Policy);
-}
+
+private:
+ const PrintingPolicy &Policy;
+};
+
+} // end anonymous namespace
std::pair
-Sema::findFailedBooleanCondition(Expr *Cond, bool AllowTopLevelCond) {
+Sema::findFailedBooleanCondition(Expr *Cond) {
Cond = lookThroughRangesV3Condition(PP, Cond);
// Separate out all of the terms in a conjunction.
@@ -3087,11 +3099,6 @@
for (Expr *Term : Terms) {
Expr *TermAsWritten = Term->IgnoreParenImpCasts();
-// Literals are uninteresting.
-if (isa(TermAsWritten) ||
-isa(TermAsWritten))
- continue;
-
// The initialization of the parameter from the argument is
// a constant-evaluated context.
EnterExpressionEvaluationContext ConstantEvaluated(
@@ -3104,18 +3111,18 @@
break;
}
}
-
- if (!FailedCond) {
-if (!AllowTopLevelCond)
- return { nullptr, "" };
-
+ if (!FailedCond)
FailedCond = Cond->IgnoreParenImpCasts();
- }
+
+ // Literals are uninteresting.
+ if (isa(FailedCond) || isa(FailedCond))
+return {nullptr, ""};
std::string Description;
{
llvm::raw_string_ostream Out(Description);
-prettyPrintFailedBooleanCondition(Out, FailedCond, getPrintingPolicy());
+FailedBooleanConditionPrinterHelper Helper(getPrintingPolicy());
+FailedCond->printPretty(Out, &Helper, getPrintingPolicy());
}
return { FailedCond, Description };
}
@@ -3199,9 +3206,7 @@
Expr *FailedCond;
std::string FailedDescription;
std::tie(FailedCond, FailedDescription) =
- findFailedBooleanCondition(
-TemplateArgs[0].getSourceExpression(),
-/*AllowTopLevelCond=*/true);
+ findFailedBooleanCondition(TemplateArgs[0].getSourceExpression());
// Remove the old SFINAE diagnostic.
PartialDiagnosticAt OldDiag =
@@ -9649,7 +9654,7 @@
Expr *FailedCond;
std::string FailedDescription;
std::tie(FailedCond, FailedDescription) =
- findFailedBooleanCondition(Cond, /*AllowTopLevelCond=*/true);
+ findFailedBooleanCondition(Cond);
Diag(FailedCond->getExprLo
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
courbet added a comment. Thanks. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55270/new/ https://reviews.llvm.org/D55270 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Comment at: test/PCH/cxx-static_assert.cpp:17
-// expected-error@12 {{static_assert failed "N is not 2!"}}
+// expected-error@12 {{static_assert failed due to requirement '1 == 2' "N is
not 2!"}}
T<1> t1; // expected-note {{in instantiation of template class 'T<1>'
requested here}}
Quuxplusone wrote:
> aaron.ballman wrote:
> > Quuxplusone wrote:
> > > courbet wrote:
> > > > aaron.ballman wrote:
> > > > > I'm not certain how I feel about now printing the failure condition
> > > > > when there's an explicit message provided. From what I understand, a
> > > > > fair amount of code in the wild does `static_assert(some_condition,
> > > > > "some_condition")` because of older language modes where the message
> > > > > was not optional. I worry we're going to start seeing a lot of
> > > > > diagnostics like: `static_assert failed due to requirement '1 == 2'
> > > > > "N == 2"`, which seems a bit low-quality. See
> > > > > `DFAPacketizer::DFAPacketizer()` in LLVM as an example of something
> > > > > similar.
> > > > >
> > > > > Given that the user entered a message, do we still want to show the
> > > > > requirement? Do we feel the same way if the requirement is fairly
> > > > > large?
> > > > The issue is that `"N == 2"` is a useless error message. Actually,
> > > > since the error message has to be a string literal, there is no way
> > > > for the user to print a debuggable output. So I really think we should
> > > > print the failed condition.
> > > FWIW, I also don't agree with Aaron's concern.
> > >
> > > I do think there is a lot of code in the wild whose string literal was
> > > "phoned in." After all, this is why C++17 allows us to omit the string
> > > literal: to avoid boilerplate like this.
> > >
> > > static_assert(some-condition, "some-condition");
> > > static_assert(some-condition, "some-condition was not satisfied");
> > > static_assert(some-condition, "some-condition must be satisfied");
> > > static_assert(some-condition, "");
> > >
> > > But should Clang go _out of its way_ to suppress such "redundant" string
> > > literals? First of all, such suppression would be C++14-and-earlier: if a
> > > C++17-native program contains a string literal, we should maybe assume
> > > it's on purpose. Second, it's not clear how a machine could detect
> > > "redundant" literals with high fidelity.
> > >
> > > static_assert(std::is_integral, "std::is_integral");
> > > // clearly redundant
> > > static_assert(std::is_integral, "T must be integral");
> > > // redundant, but unlikely to be machine-detectable as such
> > >
> > > static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 *
> > > sizeof(DFAInput)),
> > > "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) > (8 *
> > > sizeof(DFAInput))");
> > > // redundant, but unlikely to be machine-detectable as such
> > > // thanks to the substitution of > for <=
> > >
> > > static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 *
> > > sizeof(DFAInput)),
> > > "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) too big for DFAInput");
> > > // redundant, but unlikely to be machine-detectable as such
> > >
> > > In any event, I agree with @courbet that Clang should print the expansion
> > > of the failed condition in any case.
> > >
> > > Also: One might argue that if the `static_assert` fits completely on one
> > > source line, then we could omit the string-literal from our error message
> > > because the string literal will be shown anyway as part of the offending
> > > source line — but I believe IDE-users would complain about that, so, we
> > > shouldn't do that. And even then, we'd still want to print the failed
> > > condition!
> > >
> > > Checking my understanding: The `static_assert` above (taken from
> > > `DFAPacketizer::DFAPacketizer()` in LLVM) would be unchanged by
> > > @courbet's patches, because none of its subexpressions are
> > > template-dependent. Right?
> > > But should Clang go _out of its way_ to suppress such "redundant" string
> > > literals?
> >
> > I wasn't suggesting it should; I was suggesting that Clang should be
> > conservative and suppress printing the conditional when a message is
> > present, not when they look to be redundant enough.
> >
> > > if a C++17-native program contains a string literal, we should maybe
> > > assume it's on purpose.
> >
> > This is the exact scenario I was envisioning.
> >
> > It's a relatively weak preference, but I kind of prefer not displaying the
> > conditional information in the presence of a message (at least in C++17 and
> > above), especially as the conditional can be huge. I'm thinking of
> > scenarios where the user does something like:
> > ```
> > static_assert(condition1 && condition2 && (condition3 || condi
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
Quuxplusone added a subscriber: saar.raz.
Quuxplusone added inline comments.
Comment at: test/PCH/cxx-static_assert.cpp:17
-// expected-error@12 {{static_assert failed "N is not 2!"}}
+// expected-error@12 {{static_assert failed due to requirement '1 == 2' "N is
not 2!"}}
T<1> t1; // expected-note {{in instantiation of template class 'T<1>'
requested here}}
aaron.ballman wrote:
> Quuxplusone wrote:
> > courbet wrote:
> > > aaron.ballman wrote:
> > > > I'm not certain how I feel about now printing the failure condition
> > > > when there's an explicit message provided. From what I understand, a
> > > > fair amount of code in the wild does `static_assert(some_condition,
> > > > "some_condition")` because of older language modes where the message
> > > > was not optional. I worry we're going to start seeing a lot of
> > > > diagnostics like: `static_assert failed due to requirement '1 == 2' "N
> > > > == 2"`, which seems a bit low-quality. See
> > > > `DFAPacketizer::DFAPacketizer()` in LLVM as an example of something
> > > > similar.
> > > >
> > > > Given that the user entered a message, do we still want to show the
> > > > requirement? Do we feel the same way if the requirement is fairly large?
> > > The issue is that `"N == 2"` is a useless error message. Actually, since
> > > the error message has to be a string literal, there is no way for the
> > > user to print a debuggable output. So I really think we should print the
> > > failed condition.
> > FWIW, I also don't agree with Aaron's concern.
> >
> > I do think there is a lot of code in the wild whose string literal was
> > "phoned in." After all, this is why C++17 allows us to omit the string
> > literal: to avoid boilerplate like this.
> >
> > static_assert(some-condition, "some-condition");
> > static_assert(some-condition, "some-condition was not satisfied");
> > static_assert(some-condition, "some-condition must be satisfied");
> > static_assert(some-condition, "");
> >
> > But should Clang go _out of its way_ to suppress such "redundant" string
> > literals? First of all, such suppression would be C++14-and-earlier: if a
> > C++17-native program contains a string literal, we should maybe assume it's
> > on purpose. Second, it's not clear how a machine could detect "redundant"
> > literals with high fidelity.
> >
> > static_assert(std::is_integral, "std::is_integral");
> > // clearly redundant
> > static_assert(std::is_integral, "T must be integral");
> > // redundant, but unlikely to be machine-detectable as such
> >
> > static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 *
> > sizeof(DFAInput)),
> > "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) > (8 * sizeof(DFAInput))");
> > // redundant, but unlikely to be machine-detectable as such
> > // thanks to the substitution of > for <=
> >
> > static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 *
> > sizeof(DFAInput)),
> > "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) too big for DFAInput");
> > // redundant, but unlikely to be machine-detectable as such
> >
> > In any event, I agree with @courbet that Clang should print the expansion
> > of the failed condition in any case.
> >
> > Also: One might argue that if the `static_assert` fits completely on one
> > source line, then we could omit the string-literal from our error message
> > because the string literal will be shown anyway as part of the offending
> > source line — but I believe IDE-users would complain about that, so, we
> > shouldn't do that. And even then, we'd still want to print the failed
> > condition!
> >
> > Checking my understanding: The `static_assert` above (taken from
> > `DFAPacketizer::DFAPacketizer()` in LLVM) would be unchanged by @courbet's
> > patches, because none of its subexpressions are template-dependent. Right?
> > But should Clang go _out of its way_ to suppress such "redundant" string
> > literals?
>
> I wasn't suggesting it should; I was suggesting that Clang should be
> conservative and suppress printing the conditional when a message is present,
> not when they look to be redundant enough.
>
> > if a C++17-native program contains a string literal, we should maybe assume
> > it's on purpose.
>
> This is the exact scenario I was envisioning.
>
> It's a relatively weak preference, but I kind of prefer not displaying the
> conditional information in the presence of a message (at least in C++17 and
> above), especially as the conditional can be huge. I'm thinking of scenarios
> where the user does something like:
> ```
> static_assert(condition1 && condition2 && (condition3 || condition4), "Simple
> explanation");
> ```
> except that `condition` is replaced by `std::some_type_trait` in
> various interesting ways.
>
> I'm thinking of scenarios where the user does something like:
>
> static_assert(condition1 && condition2 && (condit
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
aaron.ballman added inline comments.
Comment at: test/PCH/cxx-static_assert.cpp:17
-// expected-error@12 {{static_assert failed "N is not 2!"}}
+// expected-error@12 {{static_assert failed due to requirement '1 == 2' "N is
not 2!"}}
T<1> t1; // expected-note {{in instantiation of template class 'T<1>'
requested here}}
Quuxplusone wrote:
> courbet wrote:
> > aaron.ballman wrote:
> > > I'm not certain how I feel about now printing the failure condition when
> > > there's an explicit message provided. From what I understand, a fair
> > > amount of code in the wild does `static_assert(some_condition,
> > > "some_condition")` because of older language modes where the message was
> > > not optional. I worry we're going to start seeing a lot of diagnostics
> > > like: `static_assert failed due to requirement '1 == 2' "N == 2"`, which
> > > seems a bit low-quality. See `DFAPacketizer::DFAPacketizer()` in LLVM as
> > > an example of something similar.
> > >
> > > Given that the user entered a message, do we still want to show the
> > > requirement? Do we feel the same way if the requirement is fairly large?
> > The issue is that `"N == 2"` is a useless error message. Actually, since
> > the error message has to be a string literal, there is no way for the user
> > to print a debuggable output. So I really think we should print the failed
> > condition.
> FWIW, I also don't agree with Aaron's concern.
>
> I do think there is a lot of code in the wild whose string literal was
> "phoned in." After all, this is why C++17 allows us to omit the string
> literal: to avoid boilerplate like this.
>
> static_assert(some-condition, "some-condition");
> static_assert(some-condition, "some-condition was not satisfied");
> static_assert(some-condition, "some-condition must be satisfied");
> static_assert(some-condition, "");
>
> But should Clang go _out of its way_ to suppress such "redundant" string
> literals? First of all, such suppression would be C++14-and-earlier: if a
> C++17-native program contains a string literal, we should maybe assume it's
> on purpose. Second, it's not clear how a machine could detect "redundant"
> literals with high fidelity.
>
> static_assert(std::is_integral, "std::is_integral");
> // clearly redundant
> static_assert(std::is_integral, "T must be integral");
> // redundant, but unlikely to be machine-detectable as such
>
> static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 *
> sizeof(DFAInput)),
> "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) > (8 * sizeof(DFAInput))");
> // redundant, but unlikely to be machine-detectable as such
> // thanks to the substitution of > for <=
>
> static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 *
> sizeof(DFAInput)),
> "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) too big for DFAInput");
> // redundant, but unlikely to be machine-detectable as such
>
> In any event, I agree with @courbet that Clang should print the expansion of
> the failed condition in any case.
>
> Also: One might argue that if the `static_assert` fits completely on one
> source line, then we could omit the string-literal from our error message
> because the string literal will be shown anyway as part of the offending
> source line — but I believe IDE-users would complain about that, so, we
> shouldn't do that. And even then, we'd still want to print the failed
> condition!
>
> Checking my understanding: The `static_assert` above (taken from
> `DFAPacketizer::DFAPacketizer()` in LLVM) would be unchanged by @courbet's
> patches, because none of its subexpressions are template-dependent. Right?
> But should Clang go _out of its way_ to suppress such "redundant" string
> literals?
I wasn't suggesting it should; I was suggesting that Clang should be
conservative and suppress printing the conditional when a message is present,
not when they look to be redundant enough.
> if a C++17-native program contains a string literal, we should maybe assume
> it's on purpose.
This is the exact scenario I was envisioning.
It's a relatively weak preference, but I kind of prefer not displaying the
conditional information in the presence of a message (at least in C++17 and
above), especially as the conditional can be huge. I'm thinking of scenarios
where the user does something like:
```
static_assert(condition1 && condition2 && (condition3 || condition4), "Simple
explanation");
```
except that `condition` is replaced by `std::some_type_trait` in various
interesting ways.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55270/new/
https://reviews.llvm.org/D55270
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
Quuxplusone added inline comments.
Comment at: test/PCH/cxx-static_assert.cpp:17
-// expected-error@12 {{static_assert failed "N is not 2!"}}
+// expected-error@12 {{static_assert failed due to requirement '1 == 2' "N is
not 2!"}}
T<1> t1; // expected-note {{in instantiation of template class 'T<1>'
requested here}}
courbet wrote:
> aaron.ballman wrote:
> > I'm not certain how I feel about now printing the failure condition when
> > there's an explicit message provided. From what I understand, a fair amount
> > of code in the wild does `static_assert(some_condition, "some_condition")`
> > because of older language modes where the message was not optional. I worry
> > we're going to start seeing a lot of diagnostics like: `static_assert
> > failed due to requirement '1 == 2' "N == 2"`, which seems a bit
> > low-quality. See `DFAPacketizer::DFAPacketizer()` in LLVM as an example of
> > something similar.
> >
> > Given that the user entered a message, do we still want to show the
> > requirement? Do we feel the same way if the requirement is fairly large?
> The issue is that `"N == 2"` is a useless error message. Actually, since the
> error message has to be a string literal, there is no way for the user to
> print a debuggable output. So I really think we should print the failed
> condition.
FWIW, I also don't agree with Aaron's concern.
I do think there is a lot of code in the wild whose string literal was "phoned
in." After all, this is why C++17 allows us to omit the string literal: to
avoid boilerplate like this.
static_assert(some-condition, "some-condition");
static_assert(some-condition, "some-condition was not satisfied");
static_assert(some-condition, "some-condition must be satisfied");
static_assert(some-condition, "");
But should Clang go _out of its way_ to suppress such "redundant" string
literals? First of all, such suppression would be C++14-and-earlier: if a
C++17-native program contains a string literal, we should maybe assume it's on
purpose. Second, it's not clear how a machine could detect "redundant" literals
with high fidelity.
static_assert(std::is_integral, "std::is_integral");
// clearly redundant
static_assert(std::is_integral, "T must be integral");
// redundant, but unlikely to be machine-detectable as such
static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 *
sizeof(DFAInput)),
"(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) > (8 * sizeof(DFAInput))");
// redundant, but unlikely to be machine-detectable as such
// thanks to the substitution of > for <=
static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 *
sizeof(DFAInput)),
"(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) too big for DFAInput");
// redundant, but unlikely to be machine-detectable as such
In any event, I agree with @courbet that Clang should print the expansion of
the failed condition in any case.
Also: One might argue that if the `static_assert` fits completely on one source
line, then we could omit the string-literal from our error message because the
string literal will be shown anyway as part of the offending source line — but
I believe IDE-users would complain about that, so, we shouldn't do that. And
even then, we'd still want to print the failed condition!
Checking my understanding: The `static_assert` above (taken from
`DFAPacketizer::DFAPacketizer()` in LLVM) would be unchanged by @courbet's
patches, because none of its subexpressions are template-dependent. Right?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55270/new/
https://reviews.llvm.org/D55270
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
courbet marked an inline comment as done.
courbet added inline comments.
Comment at: test/PCH/cxx-static_assert.cpp:17
-// expected-error@12 {{static_assert failed "N is not 2!"}}
+// expected-error@12 {{static_assert failed due to requirement '1 == 2' "N is
not 2!"}}
T<1> t1; // expected-note {{in instantiation of template class 'T<1>'
requested here}}
aaron.ballman wrote:
> I'm not certain how I feel about now printing the failure condition when
> there's an explicit message provided. From what I understand, a fair amount
> of code in the wild does `static_assert(some_condition, "some_condition")`
> because of older language modes where the message was not optional. I worry
> we're going to start seeing a lot of diagnostics like: `static_assert failed
> due to requirement '1 == 2' "N == 2"`, which seems a bit low-quality. See
> `DFAPacketizer::DFAPacketizer()` in LLVM as an example of something similar.
>
> Given that the user entered a message, do we still want to show the
> requirement? Do we feel the same way if the requirement is fairly large?
The issue is that `"N == 2"` is a useless error message. Actually, since the
error message has to be a string literal, there is no way for the user to print
a debuggable output. So I really think we should print the failed condition.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55270/new/
https://reviews.llvm.org/D55270
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
aaron.ballman added inline comments.
Comment at: test/PCH/cxx-static_assert.cpp:17
-// expected-error@12 {{static_assert failed "N is not 2!"}}
+// expected-error@12 {{static_assert failed due to requirement '1 == 2' "N is
not 2!"}}
T<1> t1; // expected-note {{in instantiation of template class 'T<1>'
requested here}}
I'm not certain how I feel about now printing the failure condition when
there's an explicit message provided. From what I understand, a fair amount of
code in the wild does `static_assert(some_condition, "some_condition")` because
of older language modes where the message was not optional. I worry we're going
to start seeing a lot of diagnostics like: `static_assert failed due to
requirement '1 == 2' "N == 2"`, which seems a bit low-quality. See
`DFAPacketizer::DFAPacketizer()` in LLVM as an example of something similar.
Given that the user entered a message, do we still want to show the
requirement? Do we feel the same way if the requirement is fairly large?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55270/new/
https://reviews.llvm.org/D55270
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
courbet updated this revision to Diff 176773.
courbet added a comment.
- Update PHC and C11 tests.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55270/new/
https://reviews.llvm.org/D55270
Files:
include/clang/Sema/Sema.h
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaTemplate.cpp
test/PCH/cxx-static_assert.cpp
test/Sema/static-assert.c
test/SemaCXX/static-assert-cxx17.cpp
test/SemaCXX/static-assert.cpp
Index: test/SemaCXX/static-assert.cpp
===
--- test/SemaCXX/static-assert.cpp
+++ test/SemaCXX/static-assert.cpp
@@ -15,14 +15,14 @@
};
template struct T {
-static_assert(N == 2, "N is not 2!"); // expected-error {{static_assert failed "N is not 2!"}}
+static_assert(N == 2, "N is not 2!"); // expected-error {{static_assert failed due to requirement '1 == 2' "N is not 2!"}}
};
T<1> t1; // expected-note {{in instantiation of template class 'T<1>' requested here}}
T<2> t2;
template struct S {
-static_assert(sizeof(T) > sizeof(char), "Type not big enough!"); // expected-error {{static_assert failed "Type not big enough!"}}
+static_assert(sizeof(T) > sizeof(char), "Type not big enough!"); // expected-error {{static_assert failed due to requirement 'sizeof(char) > sizeof(char)' "Type not big enough!"}}
};
S s1; // expected-note {{in instantiation of template class 'S' requested here}}
@@ -111,6 +111,14 @@
// expected-error@-1{{static_assert failed due to requirement 'std::is_same::value' "message"}}
static_assert(std::is_const::value, "message");
// expected-error@-1{{static_assert failed due to requirement 'std::is_const::value' "message"}}
+static_assert(!std::is_const::value, "message");
+// expected-error@-1{{static_assert failed due to requirement '!std::is_const::value' "message"}}
+static_assert(!(std::is_const::value), "message");
+// expected-error@-1{{static_assert failed due to requirement '!(std::is_const::value)' "message"}}
+static_assert(std::is_const::value == false, "message");
+// expected-error@-1{{static_assert failed due to requirement 'std::is_const::value == false' "message"}}
+static_assert(!(std::is_const::value == true), "message");
+// expected-error@-1{{static_assert failed due to requirement '!(std::is_const::value == true)' "message"}}
struct BI_tag {};
struct RAI_tag : BI_tag {};
Index: test/SemaCXX/static-assert-cxx17.cpp
===
--- test/SemaCXX/static-assert-cxx17.cpp
+++ test/SemaCXX/static-assert-cxx17.cpp
@@ -45,3 +45,12 @@
};
template void foo4();
// expected-note@-1{{in instantiation of function template specialization 'foo4' requested here}}
+
+
+template
+void foo5() {
+ static_assert(!!(global_inline_var));
+ // expected-error@-1{{static_assert failed due to requirement '!!(global_inline_var)'}}
+}
+template void foo5();
+// expected-note@-1{{in instantiation of function template specialization 'foo5' requested here}}
Index: test/Sema/static-assert.c
===
--- test/Sema/static-assert.c
+++ test/Sema/static-assert.c
@@ -38,5 +38,5 @@
typedef UNION(unsigned, struct A) U1;
UNION(char[2], short) u2 = { .one = { 'a', 'b' } };
-typedef UNION(char, short) U3; // expected-error {{static_assert failed "type size mismatch"}}
+typedef UNION(char, short) U3; // expected-error {{static_assert failed due to requirement 'sizeof(char) == sizeof(short)' "type size mismatch"}}
typedef UNION(float, 0.5f) U4; // expected-error {{expected a type}}
Index: test/PCH/cxx-static_assert.cpp
===
--- test/PCH/cxx-static_assert.cpp
+++ test/PCH/cxx-static_assert.cpp
@@ -3,7 +3,7 @@
// Test with pch.
// RUN: %clang_cc1 -std=c++11 -emit-pch -o %t %s
-// RUN: %clang_cc1 -include-pch %t -verify -std=c++11 %s
+// RUN: %clang_cc1 -include-pch %t -verify -std=c++11 %s
#ifndef HEADER
#define HEADER
@@ -14,7 +14,7 @@
#else
-// expected-error@12 {{static_assert failed "N is not 2!"}}
+// expected-error@12 {{static_assert failed due to requirement '1 == 2' "N is not 2!"}}
T<1> t1; // expected-note {{in instantiation of template class 'T<1>' requested here}}
T<2> t2;
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -3052,30 +3052,42 @@
return Cond;
}
-// Print a diagnostic for the failing static_assert expression. Defaults to
-// pretty-printing the expression.
-static void prettyPrintFailedBooleanCondition(llvm::raw_string_ostream &OS,
- const Expr *FailedCond,
- const PrintingPolicy &Policy) {
- const auto *DR = dyn_cast(FailedCond);
- if (DR && DR->getQualifier()) {
-// If this is a qualified name, expand the template arguments in nested
-//
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
courbet updated this revision to Diff 176771.
courbet marked 2 inline comments as done.
courbet added a comment.
- Address comments
- Add more tests
- Remove AllowTopLevel and handle cases like `sizeof(T)` (-> `sizeof(int)`)
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55270/new/
https://reviews.llvm.org/D55270
Files:
include/clang/Sema/Sema.h
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaTemplate.cpp
test/SemaCXX/static-assert-cxx17.cpp
test/SemaCXX/static-assert.cpp
Index: test/SemaCXX/static-assert.cpp
===
--- test/SemaCXX/static-assert.cpp
+++ test/SemaCXX/static-assert.cpp
@@ -15,14 +15,14 @@
};
template struct T {
-static_assert(N == 2, "N is not 2!"); // expected-error {{static_assert failed "N is not 2!"}}
+static_assert(N == 2, "N is not 2!"); // expected-error {{static_assert failed due to requirement '1 == 2' "N is not 2!"}}
};
T<1> t1; // expected-note {{in instantiation of template class 'T<1>' requested here}}
T<2> t2;
template struct S {
-static_assert(sizeof(T) > sizeof(char), "Type not big enough!"); // expected-error {{static_assert failed "Type not big enough!"}}
+static_assert(sizeof(T) > sizeof(char), "Type not big enough!"); // expected-error {{static_assert failed due to requirement 'sizeof(char) > sizeof(char)' "Type not big enough!"}}
};
S s1; // expected-note {{in instantiation of template class 'S' requested here}}
@@ -111,6 +111,14 @@
// expected-error@-1{{static_assert failed due to requirement 'std::is_same::value' "message"}}
static_assert(std::is_const::value, "message");
// expected-error@-1{{static_assert failed due to requirement 'std::is_const::value' "message"}}
+static_assert(!std::is_const::value, "message");
+// expected-error@-1{{static_assert failed due to requirement '!std::is_const::value' "message"}}
+static_assert(!(std::is_const::value), "message");
+// expected-error@-1{{static_assert failed due to requirement '!(std::is_const::value)' "message"}}
+static_assert(std::is_const::value == false, "message");
+// expected-error@-1{{static_assert failed due to requirement 'std::is_const::value == false' "message"}}
+static_assert(!(std::is_const::value == true), "message");
+// expected-error@-1{{static_assert failed due to requirement '!(std::is_const::value == true)' "message"}}
struct BI_tag {};
struct RAI_tag : BI_tag {};
Index: test/SemaCXX/static-assert-cxx17.cpp
===
--- test/SemaCXX/static-assert-cxx17.cpp
+++ test/SemaCXX/static-assert-cxx17.cpp
@@ -45,3 +45,12 @@
};
template void foo4();
// expected-note@-1{{in instantiation of function template specialization 'foo4' requested here}}
+
+
+template
+void foo5() {
+ static_assert(!!(global_inline_var));
+ // expected-error@-1{{static_assert failed due to requirement '!!(global_inline_var)'}}
+}
+template void foo5();
+// expected-note@-1{{in instantiation of function template specialization 'foo5' requested here}}
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -3052,30 +3052,42 @@
return Cond;
}
-// Print a diagnostic for the failing static_assert expression. Defaults to
-// pretty-printing the expression.
-static void prettyPrintFailedBooleanCondition(llvm::raw_string_ostream &OS,
- const Expr *FailedCond,
- const PrintingPolicy &Policy) {
- const auto *DR = dyn_cast(FailedCond);
- if (DR && DR->getQualifier()) {
-// If this is a qualified name, expand the template arguments in nested
-// qualifiers.
-DR->getQualifier()->print(OS, Policy, true);
-// Then print the decl itself.
-const ValueDecl *VD = DR->getDecl();
-OS << VD->getName();
-if (const auto *IV = dyn_cast(VD)) {
- // This is a template variable, print the expanded template arguments.
- printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
+namespace {
+
+// A PrinterHelper that prints more helpful diagnostics for some sub-expressions
+// within failing boolean expression, such as substituting template parameters
+// for actual types.
+class FailedBooleanConditionPrinterHelper : public PrinterHelper {
+public:
+ explicit FailedBooleanConditionPrinterHelper(const PrintingPolicy &Policy)
+ : Policy(Policy) {}
+
+ bool handledStmt(Stmt *E, raw_ostream &OS) override {
+const auto *DR = dyn_cast(E);
+if (DR && DR->getQualifier()) {
+ // If this is a qualified name, expand the template arguments in nested
+ // qualifiers.
+ DR->getQualifier()->print(OS, Policy, true);
+ // Then print the decl itself.
+ const ValueDecl *VD = DR->getDecl();
+ OS << VD->getName();
+ if (const auto *IV = dyn_cast(VD)) {
+// This is a templat
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
courbet marked 7 inline comments as done.
courbet added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:3065
+
+ ~FailedBooleanConditionPrinterHelper() override {}
+
Quuxplusone wrote:
> aaron.ballman wrote:
> > Is this definition necessary?
> Nit: I don't know if it is LLVM style to provide explicitly written-out
> overrides for all virtual destructors. In my own code, if a destructor would
> be redundant, I wouldn't write it.
I don't think the style guide says anything; removed.
Comment at: test/SemaCXX/static-assert.cpp:117
+static_assert(!(std::is_const::value), "message");
+// expected-error@-1{{static_assert failed due to requirement
'!(std::is_const::value)' "message"}}
Quuxplusone wrote:
> Please also add a test case for the `is_const_v` inline-variable-template
> version.
> ```
> template
> inline constexpr bool is_const_v = is_const::value;
> static_assert(is_const_v, "message"); // if this test case
> was missing from the previous patch
> static_assert(!is_const_v, "message"); // exercise the
> same codepath for this new feature
> ```
>
> Also, does using the PrinterHelper mean that you get a bunch of other cases
> for free? Like, does this work now too?
> ```
> static_assert(is_const::value == false, "message");
> ```
> Please also add a test case for the is_const_v inline-variable-template
> version.
done (in c++17 tests)
> Also, does using the PrinterHelper mean that you get a bunch of other cases
> for free? Like, does this work now too?
Exactly. While I'm at it I've added tests for your use case and removed the no
longer needed AllowTopLevel parameter.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55270/new/
https://reviews.llvm.org/D55270
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
Quuxplusone added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:3062
+public:
+ FailedBooleanConditionPrinterHelper(const PrintingPolicy &Policy)
+ : Policy(Policy) {}
`explicit`
Comment at: lib/Sema/SemaTemplate.cpp:3065
+
+ ~FailedBooleanConditionPrinterHelper() override {}
+
aaron.ballman wrote:
> Is this definition necessary?
Nit: I don't know if it is LLVM style to provide explicitly written-out
overrides for all virtual destructors. In my own code, if a destructor would be
redundant, I wouldn't write it.
Comment at: lib/Sema/SemaTemplate.cpp:3089
+
+} // namespace
`} // end anonymous namespace`
Comment at: test/SemaCXX/static-assert.cpp:117
+static_assert(!(std::is_const::value), "message");
+// expected-error@-1{{static_assert failed due to requirement
'!(std::is_const::value)' "message"}}
Please also add a test case for the `is_const_v` inline-variable-template
version.
```
template
inline constexpr bool is_const_v = is_const::value;
static_assert(is_const_v, "message"); // if this test case
was missing from the previous patch
static_assert(!is_const_v, "message"); // exercise the
same codepath for this new feature
```
Also, does using the PrinterHelper mean that you get a bunch of other cases for
free? Like, does this work now too?
```
static_assert(is_const::value == false, "message");
```
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55270/new/
https://reviews.llvm.org/D55270
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
aaron.ballman added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:3065
+
+ ~FailedBooleanConditionPrinterHelper() override {}
+
Is this definition necessary?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55270/new/
https://reviews.llvm.org/D55270
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.
courbet created this revision.
courbet added reviewers: aaron.ballman, Quuxplusone.
We're now handling cases like `static_assert(!expr)` and
static_assert(!(expr))`.
Repository:
rC Clang
https://reviews.llvm.org/D55270
Files:
lib/Sema/SemaTemplate.cpp
test/SemaCXX/static-assert.cpp
Index: test/SemaCXX/static-assert.cpp
===
--- test/SemaCXX/static-assert.cpp
+++ test/SemaCXX/static-assert.cpp
@@ -111,6 +111,10 @@
// expected-error@-1{{static_assert failed due to requirement
'std::is_same::value' "message"}}
static_assert(std::is_const::value, "message");
// expected-error@-1{{static_assert failed due to requirement
'std::is_const::value' "message"}}
+static_assert(!std::is_const::value, "message");
+// expected-error@-1{{static_assert failed due to requirement
'!std::is_const::value' "message"}}
+static_assert(!(std::is_const::value), "message");
+// expected-error@-1{{static_assert failed due to requirement
'!(std::is_const::value)' "message"}}
struct BI_tag {};
struct RAI_tag : BI_tag {};
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -3052,27 +3052,41 @@
return Cond;
}
-// Print a diagnostic for the failing static_assert expression. Defaults to
-// pretty-printing the expression.
-static void prettyPrintFailedBooleanCondition(llvm::raw_string_ostream &OS,
- const Expr *FailedCond,
- const PrintingPolicy &Policy) {
- const auto *DR = dyn_cast(FailedCond);
- if (DR && DR->getQualifier()) {
-// If this is a qualified name, expand the template arguments in nested
-// qualifiers.
-DR->getQualifier()->print(OS, Policy, true);
-// Then print the decl itself.
-const ValueDecl *VD = DR->getDecl();
-OS << VD->getName();
-if (const auto *IV = dyn_cast(VD)) {
- // This is a template variable, print the expanded template arguments.
- printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
+namespace {
+
+// A PrinterHelper that prints more helpful diagnostics for some
sub-expressions
+// within failing boolean expression, such as substituting template parameters
+// for actual types.
+class FailedBooleanConditionPrinterHelper : public PrinterHelper {
+public:
+ FailedBooleanConditionPrinterHelper(const PrintingPolicy &Policy)
+ : Policy(Policy) {}
+
+ ~FailedBooleanConditionPrinterHelper() override {}
+
+ bool handledStmt(Stmt *E, raw_ostream &OS) override {
+const auto *DR = dyn_cast(E);
+if (DR && DR->getQualifier()) {
+ // If this is a qualified name, expand the template arguments in nested
+ // qualifiers.
+ DR->getQualifier()->print(OS, Policy, true);
+ // Then print the decl itself.
+ const ValueDecl *VD = DR->getDecl();
+ OS << VD->getName();
+ if (const auto *IV = dyn_cast(VD)) {
+// This is a template variable, print the expanded template arguments.
+printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
+ }
+ return true;
}
-return;
+return false;
}
- FailedCond->printPretty(OS, nullptr, Policy);
-}
+
+private:
+ const PrintingPolicy &Policy;
+};
+
+} // namespace
std::pair
Sema::findFailedBooleanCondition(Expr *Cond, bool AllowTopLevelCond) {
@@ -3115,7 +3129,8 @@
std::string Description;
{
llvm::raw_string_ostream Out(Description);
-prettyPrintFailedBooleanCondition(Out, FailedCond, getPrintingPolicy());
+FailedBooleanConditionPrinterHelper Helper(getPrintingPolicy());
+FailedCond->printPretty(Out, &Helper, getPrintingPolicy());
}
return { FailedCond, Description };
}
Index: test/SemaCXX/static-assert.cpp
===
--- test/SemaCXX/static-assert.cpp
+++ test/SemaCXX/static-assert.cpp
@@ -111,6 +111,10 @@
// expected-error@-1{{static_assert failed due to requirement 'std::is_same::value' "message"}}
static_assert(std::is_const::value, "message");
// expected-error@-1{{static_assert failed due to requirement 'std::is_const::value' "message"}}
+static_assert(!std::is_const::value, "message");
+// expected-error@-1{{static_assert failed due to requirement '!std::is_const::value' "message"}}
+static_assert(!(std::is_const::value), "message");
+// expected-error@-1{{static_assert failed due to requirement '!(std::is_const::value)' "message"}}
struct BI_tag {};
struct RAI_tag : BI_tag {};
Index: lib/Sema/SemaTemplate.cpp
===
--- lib/Sema/SemaTemplate.cpp
+++ lib/Sema/SemaTemplate.cpp
@@ -3052,27 +3052,41 @@
return Cond;
}
-// Print a diagnostic for the failing static_assert expression. Defaults to
-// pretty-printing the expression.
-static void prettyPrintFailedBooleanCondition(llvm
