[clang][Diagnostics] Refactor printableTextForNextCharacter

[tbaeder]

Summary

Rename parameters and local variables to start with an uppercase characters.

This is not 100% NFC, since it adds an easy/fast path for 1 byte printable characters and changes how much text is converted to UTF32.

[tbaeder]

clang/lib/Frontend/TextDiagnostic.cpp

120

I don't know what this computation of end means, but from the debug output I added, it meant that a call to this function always converted the entire line. I think.

[cor3ntin]

This generally looks good to me but i have a couple remarks

clang/lib/Frontend/TextDiagnostic.cpp

119–123

this could be simplified : the common case for ascii could be just looking at isPrint(*Begin); (which implies CharSize == 1 and llvm::isLegalUTF8Sequence(Begin, End))
So you could do it before computing CharSize

122

I think we should check that CharSize is not greater than Begin + SourceLine.size() - *i or something along those lines otherwise we may overflow SourceLine.

[tahonermann]

Splitting this into two patches (one to do the renames, another to perform the other changes) would simplify review.

clang/lib/Frontend/TextDiagnostic.cpp

117–118

120

This could assign End to a location past the end of the source line when the source line ends with an ill-formed code unit sequence (e.g., a truncated 4-byte sequence). Constructing such a pointer is likely to get one in trouble.

120

It looks to me like an overly complicated way of asking for SourceLine.bytes_end().

[tbaeder]

clang/lib/Frontend/TextDiagnostic.cpp

120

If we use bytes_end() (unconditionally) we will check the entire source line every time we call this function (which always happens in a loop), which is what I was trying to avoid.

[tbaeder]

clang/lib/Frontend/TextDiagnostic.cpp

119–123

This is not true in my testing fwiw.

[cor3ntin]

clang/lib/Frontend/TextDiagnostic.cpp

119–123

const unsigned char *Begin = SourceLine.bytes_begin() + *I;

  // Fast path for the common ASCII case.
  if (*Begin < 0x80 && llvm::sys::locale::isPrint(*Begin)) {
    ++(*I);
    return std::make_pair(SmallString<16>(Begin, Begin + 1), true);
  }

seems to work fine locally. Note that I'm not sure *Begin is always valid - it seems to be, but we might want an assert to check that SourceLine is not empty.

[tbaeder]

clang/lib/Frontend/TextDiagnostic.cpp

119–123

This function is only ever called in a while (I < SourceLine.size()) loop. I've thought about refactoring this into a helper struct that keeps the index separate from the calling function to simplify callers.

[tbaeder]

clang/lib/Frontend/TextDiagnostic.cpp

119–123

Oh also, there are two asserts at the beginning of the function to ensure I is valid.

[cor3ntin]

Give some time for @tahonermann to have an opportunity to review again, but otherwise this looks good to me. Thanks!

clang/lib/Frontend/TextDiagnostic.cpp

119–123

you are right, if, SourceLine is empty, that first asset would always fire

[tbaeder]

Ping

[tbaeder]

Ping @tahonermann

[tahonermann]

The changes look good to me. I suggested two minor edits to align with parameter name changes.

The summary states that the changes are not quite NFC. In that case, we would ideally have a test that demonstrates the changed behavior. Would adding such a test be challenging?

clang/lib/Frontend/TextDiagnostic.cpp

94

101–102

[tbaeder]

In D150843#4401189, @tahonermann wrote:

The summary states that the changes are not quite NFC. In that case, we would ideally have a test that demonstrates the changed behavior. Would adding such a test be challenging?

They are NFC, it's just not 100% only a refactoring, since it adds the new ASCII-only case.

[tahonermann]

They are NFC, it's just not 100% only a refactoring, since it adds the new ASCII-only case.

Ok, thanks. Changes look good. I noticed one comment that appears to be incorrect; please just remove it when committing the changes.

clang/lib/Frontend/TextDiagnostic.cpp

128

This comment is not correct; Begin might still point to a non-printable character with a code point value less than 0x80. I suggest just removing the comment.

clang/lib/Frontend/TextDiagnostic.cpp

 ///  printable, escaped representation of the character is returned, along with
 ///  'false'. Otherwise a printable, escaped representation of the next byte
 ///  is returned along with 'false'.
 ///
 /// \note The index is updated to be used with a subsequent call to
 ///        printableTextForNextCharacter.
 ///
 /// \param SourceLine The line of source
-/// \param i Pointer to byte index,
+/// \param I Pointer to byte index,
 /// \param TabStop used to expand tabs
 /// \return pair(printable text, 'true' iff original text was printable)
 ///
 static std::pair<SmallString<16>, bool>
-printableTextForNextCharacter(StringRef SourceLine, size_t *i,
+printableTextForNextCharacter(StringRef SourceLine, size_t *I,
                               unsigned TabStop) {
-  assert(i && "i must not be null");
-  assert(*i<SourceLine.size() && "must point to a valid index");
+  assert(I && "I must not be null");
+  assert(*I < SourceLine.size() && "must point to a valid index");
 
-  if (SourceLine[*i]=='\t') {
+  if (SourceLine[*I] == '\t') {
     assert(0 < TabStop && TabStop <= DiagnosticOptions::MaxTabStop &&
            "Invalid -ftabstop value");
-    unsigned col = bytesSincePreviousTabOrLineBegin(SourceLine, *i);
-    unsigned NumSpaces = TabStop - col%TabStop;
+    unsigned Col = bytesSincePreviousTabOrLineBegin(SourceLine, *I);
+    unsigned NumSpaces = TabStop - (Col % TabStop);
     assert(0 < NumSpaces && NumSpaces <= TabStop
            && "Invalid computation of space amt");
-    ++(*i);
+    ++(*I);
 
-    SmallString<16> expandedTab;
-    expandedTab.assign(NumSpaces, ' ');
-    return std::make_pair(expandedTab, true);
-  }
-
-  unsigned char const *begin, *end;
-  begin = reinterpret_cast<unsigned char const *>(&*(SourceLine.begin() + *i));
-  end = begin + (SourceLine.size() - *i);
-
-  if (llvm::isLegalUTF8Sequence(begin, end)) {
-    llvm::UTF32 c;
-    llvm::UTF32 *cptr = &c;
-    unsigned char const *original_begin = begin;
-    unsigned char const *cp_end =
-        begin + llvm::getNumBytesForUTF8(SourceLine[*i]);
-
-    llvm::ConversionResult res = llvm::ConvertUTF8toUTF32(
-        &begin, cp_end, &cptr, cptr + 1, llvm::strictConversion);
-    (void)res;
-    assert(llvm::conversionOK == res);
-    assert(0 < begin-original_begin
-           && "we must be further along in the string now");
-    *i += begin-original_begin;
-
-    if (!llvm::sys::locale::isPrint(c)) {
-      // If next character is valid UTF-8, but not printable
-      SmallString<16> expandedCP("<U+>");
-      while (c) {
-        expandedCP.insert(expandedCP.begin()+3, llvm::hexdigit(c%16));
-        c/=16;
-      }
-      while (expandedCP.size() < 8)
-        expandedCP.insert(expandedCP.begin()+3, llvm::hexdigit(0));
-      return std::make_pair(expandedCP, false);
-    }
-
-    // If next character is valid UTF-8, and printable
-    return std::make_pair(SmallString<16>(original_begin, cp_end), true);
-
-  }
-
-  // If next byte is not valid UTF-8 (and therefore not printable)
-  SmallString<16> expandedByte("<XX>");
-  unsigned char byte = SourceLine[*i];
-  expandedByte[1] = llvm::hexdigit(byte / 16);
-  expandedByte[2] = llvm::hexdigit(byte % 16);
-  ++(*i);
-  return std::make_pair(expandedByte, false);
+    SmallString<16> ExpandedTab;
+    ExpandedTab.assign(NumSpaces, ' ');
+    return std::make_pair(ExpandedTab, true);
+  }
+
+  const unsigned char *Begin = SourceLine.bytes_begin() + *I;
+
+  // Fast path for the common ASCII case.
+  if (*Begin < 0x80 && llvm::sys::locale::isPrint(*Begin)) {
+    ++(*I);
+    return std::make_pair(SmallString<16>(Begin, Begin + 1), true);
+  }
+  unsigned CharSize = llvm::getNumBytesForUTF8(*Begin);
+  const unsigned char *End = Begin + CharSize;
+
+  // Convert it to UTF32 and check if it's printable.
+  if (End <= SourceLine.bytes_end() && llvm::isLegalUTF8Sequence(Begin, End)) {
+    llvm::UTF32 C;
+    llvm::UTF32 *CPtr = &C;
+
+    // Begin and end before conversion.
+    unsigned char const *OriginalBegin = Begin;
+    llvm::ConversionResult Res = llvm::ConvertUTF8toUTF32(
+        &Begin, End, &CPtr, CPtr + 1, llvm::strictConversion);
+    (void)Res;
+    assert(Res == llvm::conversionOK);
+    assert(OriginalBegin < Begin);
+    assert((Begin - OriginalBegin) == CharSize);
+
+    (*I) += (Begin - OriginalBegin);
+
+    // Valid, multi-byte, printable UTF8 character.
+    if (llvm::sys::locale::isPrint(C))
+      return std::make_pair(SmallString<16>(OriginalBegin, End), true);
+
+    // Valid but not printable.
+    SmallString<16> Str("<U+>");
+    while (C) {
+      Str.insert(Str.begin() + 3, llvm::hexdigit(C % 16));
+      C /= 16;
+    }
+    while (Str.size() < 8)
+      Str.insert(Str.begin() + 3, llvm::hexdigit(0));
+    return std::make_pair(Str, false);
+  }
+
+  // Otherwise, not printable since it's not valid UTF8.
+  SmallString<16> ExpandedByte("<XX>");
+  unsigned char Byte = SourceLine[*I];
+  ExpandedByte[1] = llvm::hexdigit(Byte / 16);
+  ExpandedByte[2] = llvm::hexdigit(Byte % 16);
+  ++(*I);
+  return std::make_pair(ExpandedByte, false);
 }
 
 static void expandTabs(std::string &SourceLine, unsigned TabStop) {
   size_t I = SourceLine.size();
   while (I > 0) {
     I--;
     if (SourceLine[I] != '\t')
       continue;