[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.

[Clement Courbet via Phabricator via cfe-commits]

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<const ExampleTypes::T>::value), "message");
+// expected-error at -1{{static_assert failed due to requirement '!(std::is_const<const int>::value)' "message"}}
 
----------------
Quuxplusone wrote:
> Please also add a test case for the `is_const_v` inline-variable-template version.
> ```
> template<class T>
> inline constexpr bool is_const_v = is_const<T>::value;
> static_assert(is_const_v<ExampleTypes::T>, "message");  // if this test case was missing from the previous patch
> static_assert(!is_const_v<ExampleTypes::ConstT>, "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<T>::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