[PATCH] D127923: [Diagnostics] Accept newline and format diag opts on first line

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Thanks for updating the summary.

I'm not too confident around diagnostics though.
I'm going to piggyback on @scott.linder's comments. I think they are on the point.

nit: I'm not sure if `size_t` is a thing in c++, `std::size_t` is though.



================
Comment at: clang/lib/Basic/Diagnostic.cpp:815
     for (char c : S) {
-      if (llvm::sys::locale::isPrint(c) || c == '\t') {
+      if (llvm::sys::locale::isPrint(c) || c == '\t' || c == '\n') {
         OutStr.push_back(c);
----------------
Should we account for `'\r'`-s as well? I'm not sure how it works though


================
Comment at: clang/lib/Frontend/TextDiagnosticPrinter.cpp:119-135
+  // If the diagnostic message is formatted into multiple lines, split the
+  // first line and feed it into printDiagnosticOptions so that we print the
+  // options only on the first line instead of the last line.
+  SmallString<100> InitStr;
+  size_t NewlinePos = OutStr.find_first_of('\n');
+  if (NewlinePos != StringRef::npos) {
+    for (size_t I = 0; I != NewlinePos; ++I)
----------------
scott.linder wrote:
> Nit: I'd remove the `if`s and just use the behavior of `slice(npos, ...)` returning the empty string
> 
> I also would reword "split the first line and feed it into printDiagnosticOptions", as it could be interpreted as meaning the first line is an input to `printDiagnosticOptions`
Use braces for both branches or no braces at all. [[ https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements | LLVM CodingStandards ]]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127923