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

Erich Keane via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jun 27 09:38:38 PDT 2022

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?

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list