[PATCH] D108469: Improve handling of static assert messages.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Patch application is still failing the precommit CI; it'd be nice to get that corrected for some early test feedback.



================
Comment at: clang/docs/ReleaseNotes.rst:275
   This fixes `Issue 55962 <https://github.com/llvm/llvm-project/issues/55962>`_.
+- Printable Unicode characters within `static_assert` messages are no longer
+  escaped.
----------------



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16592
+        StringLiteral *MsgStr = cast<StringLiteral>(AssertMessage);
+        if (MsgStr->getCharByteWidth() == 1)
+          Msg << MsgStr->getString();
----------------
cor3ntin wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > Do we want to use something like `isASCII()` here instead of just checking for a single-byte width? e.g., pascal strings are single byte width, but probably not something we want printed this way.
> > > 
> > > Another question is, do we want to do something slightly slower (since we know we're already on the failure path, the performance here shouldn't matter overly much) by checking `!containsNonAsciiOrNull()`?
> > > Do we want to use something like isASCII() here instead of just checking for a single-byte width? e.g., pascal strings are single byte width, but probably not something we want printed this way.
> > 
> > Maybe yes. I though about supporting `u8`, but we should definitively not make effort for string with a prefix
> > 
> > > Another question is, do we want to do something slightly slower (since we know we're already on the failure path, the performance here shouldn't matter overly much) by checking !containsNonAsciiOrNull()
> > 
> > No, we want to render unicode characters properly.
> Done.
> with this change
> 
> 
> ```
> static_assert(false, "Ω");      // outputs Ω 
> static_assert(false, u8"Ω");  // ouputs u8"\316\251"
> ```
> 
> I think this makes sense. Or rather using `u8` in static)assert makes no sense so it's not an issue, even a good thing not to have any special handling. disuade people from using prefixes in that context.
>>Another question is, do we want to do something slightly slower (since we know we're already on the failure path, the performance here shouldn't matter overly much) by checking !containsNonAsciiOrNull()
> No, we want to render unicode characters properly.

Ah derp, good point.

Thanks for the switch to `isASCII()` -- I agree that we shouldn't go to great lengths to support prefixes there until WG21 decides what to do with compile-time strings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108469