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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
