[PATCH] D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

The code looks correct to me, although I have a few suggestions inline.  When you resubmit, please include full context (e.g., `git diff -U999999`).

Jan and I discussed this ahead of time and I agree with adding the assertion as the message.  However, I'd rather hear from others before signing off.



================
Comment at: lib/Sema/SemaDeclCXX.cpp:13739-13740
       SmallString<256> MsgBuffer;
-      llvm::raw_svector_ostream Msg(MsgBuffer);
-      if (AssertMessage)
-        AssertMessage->printPretty(Msg, nullptr, getPrintingPolicy());
+      {
+        llvm::raw_svector_ostream Msg(MsgBuffer);
+        if (AssertMessage)
----------------
Is this extra nesting necessary?


================
Comment at: lib/Sema/SemaDeclCXX.cpp:13744-13746
+          AssertExpr->printPretty(Msg, nullptr, getPrintingPolicy());
+          MsgBuffer.insert(MsgBuffer.begin(), '\"');
+          MsgBuffer.append(1, '\"');
----------------
Inserting to the beginning of the buffer seems a bit weird, especially since you're jumping between `Msg` and `MsgBuffer`.

This feels more natural to me:

```
Msg << '"';
AssertExpr->printPretty(Msg, nullptr, getPrintingPolicy());
Msg << '"';
```


================
Comment at: lib/Sema/SemaDeclCXX.cpp:13761
         Diag(StaticAssertLoc, diag::err_static_assert_failed)
-          << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
+          << MsgBuffer.empty() << MsgBuffer.str()
+          << AssertExpr->getSourceRange();
----------------
Is there always a message now?  If so, can you just assert that before the `if` statement and simplify this code?


Repository:
  rC Clang

https://reviews.llvm.org/D46834