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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
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();
----------------
aaron.ballman wrote:
> 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).
I might prefer `BeginItr`, but I don't see us needing `Begin` for other things later, so I don't care much.


================
Comment at: clang/lib/Basic/Diagnostic.cpp:807
 
+static void pushEscapedString(StringRef Str, SmallVectorImpl<char> &OutStr) {
+  OutStr.reserve(OutStr.size() + Str.size());
----------------
could we have some comment as to what this is for/what it is supposed to do?  It isn't clear to me what the arguments mean here.  The name 'Str' isn't particularly helpful.


================
Comment at: clang/lib/Basic/Diagnostic.cpp:843
+      }
+      OutStr.append(Number.begin(), Number.end());
+      continue;
----------------
I find myself wondering if OutStr here should be just a stream as passed in (or immediately become one above?).


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16592
+        const auto *MsgStr = cast<StringLiteral>(AssertMessage);
+        if (MsgStr->isAscii() == 1)
+          Msg << MsgStr->getString();
----------------
why `isAscii`, which returns bool, compared to 1?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108469