[clang][Diagnostics][NFC] Remove unnecessary StringRef

[tbaeder]

Summary

Seems unnecessary to create a StringRef here just so we can drop the trailing null bytes. We can do that with the std::string we create anyway.

[aaron.ballman]

LGTM, but it's worth noting that std::string::pop_back() calls erase() and there's no guarantee that there's not an extra allocation involved as a result. However, I've not seen evidence that STLs actually do an allocation (looking at libc++ and MSVC STL, they don't appear to allocate), so I think this is fine.

[tbaeder]

In D151300#4367884, @aaron.ballman wrote:

LGTM, but it's worth noting that std::string::pop_back() calls erase() and there's no guarantee that there's not an extra allocation involved as a result. However, I've not seen evidence that STLs actually do an allocation (looking at libc++ and MSVC STL, they don't appear to allocate), so I think this is fine.

Is the allocation only relevant for performance reasons or something else?

[aaron.ballman]

In D151300#4367939, @tbaeder wrote:

In D151300#4367884, @aaron.ballman wrote:

LGTM, but it's worth noting that std::string::pop_back() calls erase() and there's no guarantee that there's not an extra allocation involved as a result. However, I've not seen evidence that STLs actually do an allocation (looking at libc++ and MSVC STL, they don't appear to allocate), so I think this is fine.

Is the allocation only relevant for performance reasons or something else?

Performance was my only concern there.

[tbaeder]

In D151300#4367943, @aaron.ballman wrote:

In D151300#4367939, @tbaeder wrote:

In D151300#4367884, @aaron.ballman wrote:

LGTM, but it's worth noting that std::string::pop_back() calls erase() and there's no guarantee that there's not an extra allocation involved as a result. However, I've not seen evidence that STLs actually do an allocation (looking at libc++ and MSVC STL, they don't appear to allocate), so I think this is fine.

Is the allocation only relevant for performance reasons or something else?

Performance was my only concern there.

Okay, good. I don't think that's too bad since the common line of code has exactly 0 trailing null bytes anyway.

clang/lib/Frontend/TextDiagnostic.cpp

for (unsigned LineNo = Lines.first; LineNo != Lines.second + 1;
     while (*LineEnd != '\n' && *LineEnd != '\r' && LineEnd != BufEnd)
       ++LineEnd;
 
     // Arbitrarily stop showing snippets when the line is too long.
     // FIXME: Don't print any lines in this case.
     if (size_t(LineEnd - LineStart) > MaxLineLengthToPrint)
       return;
 
-    // Trim trailing null-bytes.
-    StringRef Line(LineStart, LineEnd - LineStart);
-    while (!Line.empty() && Line.back() == '\0' &&
-           (LineNo != CaretLineNo || Line.size() > CaretColNo))
-      Line = Line.drop_back();
-
     // Copy the line of code into an std::string for ease of manipulation.
-    std::string SourceLine(Line.begin(), Line.end());
+    std::string SourceLine(LineStart, LineEnd);
+    // Remove trailing null bytes.
+    while (!SourceLine.empty() && SourceLine.back() == '\0' &&
+           (LineNo != CaretLineNo || SourceLine.size() > CaretColNo))
+      SourceLine.pop_back();
 
     // Build the byte to column map.
     const SourceColumnMap sourceColMap(SourceLine, DiagOpts->TabStop);
 
     std::string CaretLine;
     // Highlight all of the characters covered by Ranges with ~ characters.
     for (const auto &I : Ranges)
       highlightRange(I, LineNo, FID, sourceColMap, CaretLine, SM, LangOpts);