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

[Scott Linder via Phabricator via cfe-commits]

scott.linder added a comment.

Thank you for the patch! I will leave it to others with more knowledge of the history of diagnostics to comment on whether the premise is acceptable, but I have a few technical comments



================
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)
----------------
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`


================
Comment at: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp:99-103
+      size_t NewlinePos = PD->getShortDescription().find_first_of('\n');
+      if (NewlinePos != std::string::npos)
+        OutStr = PD->getShortDescription().str().insert(NewlinePos, WarningMsg);
+      else
+        OutStr = (PD->getShortDescription() + WarningMsg).str();
----------------
It's frustrating that `std::string::insert` doesn't follow the pattern of `npos==end-of-string`, but I'd still suggest only doing the actual insert/append once


================
Comment at: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp:100
+      size_t NewlinePos = PD->getShortDescription().find_first_of('\n');
+      if (NewlinePos != std::string::npos)
+        OutStr = PD->getShortDescription().str().insert(NewlinePos, WarningMsg);
----------------
I believe they are guaranteed to be equivalent, but this should be `StringRef::npos` or you should allocate the `std::string` sooner


================
Comment at: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp:101
+      if (NewlinePos != std::string::npos)
+        OutStr = PD->getShortDescription().str().insert(NewlinePos, WarningMsg);
+      else
----------------
I think you incur an extra copy assignment here, as the `insert` returns an lvalue-reference


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127923