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

[Corentin Jabot via Phabricator via llvm-commits]

cor3ntin added inline comments.


================
Comment at: clang/lib/Basic/Diagnostic.cpp:821
+      llvm::UTF32 CodepointValue;
+      llvm::UTF32 *CpPtr = &CodepointValue;
+      const unsigned char *CodepointBegin = Begin;
----------------
aaron.ballman wrote:
> We don't really need this variable, we can use `&CodepointValue` in the two places it's needed.
We do need a `llvm::UTF32**` though - that's why there is this otherwise not useful variable


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16592
+        StringLiteral *MsgStr = cast<StringLiteral>(AssertMessage);
+        if (MsgStr->getCharByteWidth() == 1)
+          Msg << MsgStr->getString();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108469