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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D108469#2957652 <https://reviews.llvm.org/D108469#2957652>, @jfb wrote:

> I worry that changing the general `static_assert` printing (adding a colon, and dropping the quotes) will get @hwright's law to drop on us. We can try and see if e.g. users of clang have automated checks for `static_assert` in their CI pipelines or something. I think your new format looks better, but Hyrum is finicky that way... What do others think?

I think it's fine to change; we've never promised diagnostic formatting compatibility between versions. I'm sure *someone* is relying on this somewhere, but I'm not worried we're going to break a ton of people -- hopefully enough folks are tracking trunk that we can find any major issues before releasing.

Btw, it looks like the CI is currently failing the LLVM unit tests in interesting ways. That should be resolved.

There are changes in `clang/test/Lexer/null-character-in-literal.c` but Phab is unhelpful about showing what those changes are because it thinks the file is a binary file. Can you explain what's been changed there?



================
Comment at: clang/lib/Basic/Diagnostic.cpp:791
 
+static void PushEscapedString(StringRef Str, SmallVectorImpl<char> &OutStr) {
+  OutStr.reserve(OutStr.size() + Str.size());
----------------
Per coding conventions.


================
Comment at: clang/lib/Basic/Diagnostic.cpp:793
+  OutStr.reserve(OutStr.size() + Str.size());
+  auto it = reinterpret_cast<const unsigned char *>(Str.data());
+  auto end = it + Str.size();
----------------
Please fix the clang-tidy and clang-format warnings.

Also, I think `it` is more commonly considered the name for an iterator, which this sort of isn't. I'd recommend going with `Begin` and `End` for names (that also fixes the coding style nit with the names).


================
Comment at: clang/lib/Basic/Diagnostic.cpp:803-806
+      llvm::UTF32 c;
+      llvm::UTF32 *cptr = &c;
+      unsigned char const *start = it;
+      unsigned char const *cp_end = it + llvm::getNumBytesForUTF8(*it);
----------------
You should fix these names up for the coding conventions (same suggestion applies elsewhere). Also, the local style is `const unsigned char *`.


================
Comment at: clang/lib/Basic/Diagnostic.cpp:819
+      stream << "<U+" << llvm::format_hex_no_prefix(c, 4, true) << ">";
+      OutStr.append(number.begin(), number.end());
+      continue;
----------------
I'm pretty sure you need to call `flush()` before using `number`.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16361-16363
+        if (StringLiteral *MsgStr = cast<StringLiteral>(AssertMessage)) {
+          Msg << MsgStr->getString();
+        }
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108469