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

[Arthur O'Dwyer via Phabricator via cfe-commits]

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<const ExampleTypes::T>::value), "message");
+// expected-error at -1{{static_assert failed due to requirement '!(std::is_const<const int>::value)' "message"}}
 
----------------
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");
```


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55270/new/

https://reviews.llvm.org/D55270