[clang-format] Remove duplciate code from Invalid BOM detection

[MyDeveloperDay]

Summary

Review comments on D68767: [clang-format] NFC - Move functionality into functions to help code structure asked that this duplicated code in clang-format was moved to one central location that being SourceManager (where it had originally be copied from I assume)

Moved function into static function ContentCache::getInvalidBOM(...) - (closest class to where it was defined before)
Updated clang-format to call this static function

Added unit tests for said new function in BasicTests

Sorry not my normal code area so may have the wrong reviewers. (but your names were on the recent history)

[owenpan]

Thank you for taking care of this! When I fixed a bug in this part of the code last time, I had to do it in both files.

[owenpan]

clang/include/clang/Basic/SourceManager.h

232

Can you make the parameter const?

clang/lib/Basic/SourceManager.cpp

98

Add const to the parameter to be more precise?

[MyDeveloperDay]

Updating with review comments

[klimek]

clang/include/clang/Basic/SourceManager.h

232

StringRef is inherently const though, right? I think the added const is confusing.

[MyDeveloperDay]

I tend to agree mainly given that StringRef is used without const elsewhere. I'll bow to better judgement.

[owenpan]

clang/include/clang/Basic/SourceManager.h

232

Oops! I meant const StringRef &:

static const char *getInvalidBOM(const StringRef &BufStr);

[owenpan]

Here are some [[ https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp#L229 | const StringRef & examples ]].

[klimek]

In D68914#1709002, @owenpan wrote:

Here are some [[ https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp#L229 | const StringRef & examples ]].

StringRef already is a const pointer type, so the idiomatic way is to pass it by value.

[owenpan]

I did a quick search of the LLVM code base and found dozens of const StringRef &, including [[ https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/ADT/Twine.h#L524 | the Twine API ]].

[dexonsmith]

In D68914#1710407, @owenpan wrote:

I did a quick search of the LLVM code base and found dozens of const StringRef &, including [[ https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/ADT/Twine.h#L524 | the Twine API ]].

Twine is "special", don't be influenced by that. Most of the others are probably mistakes. It's idiomatic to pass StringRef by value.

[owenpan]

Thanks for clearing it up!

[MyDeveloperDay]

I think we agree it should be (StringRef ), no const,no reference

[owenpan]

LGTM

clang/include/clang/Basic/SourceManager.h

public:
     bool isBufferInvalid() const {
       return Buffer.getInt() & InvalidFlag;
     }
 
     /// Determine whether the buffer should be freed.
     bool shouldFreeBuffer() const {
       return (Buffer.getInt() & DoNotFreeFlag) == 0;
     }
+
+    // If BufStr has an invalid BOM, returns the BOM name; otherwise, returns
+    // nullptr
+    static const char *getInvalidBOM(StringRef BufStr);
   };
 
   // Assert that the \c ContentCache objects will always be 8-byte aligned so
   // that we can pack 3 bits of integer into pointers to such objects.
   static_assert(alignof(ContentCache) >= 8,
                 "ContentCache must be 8-byte aligned.");
 
   /// Information about a FileID, basically just the logical file

clang/lib/Basic/SourceManager.cpp

void ContentCache::replaceBuffer(const llvm::MemoryBuffer *B, bool DoNotFree) {
   }
 
   if (shouldFreeBuffer())
     delete Buffer.getPointer();
   Buffer.setPointer(B);
   Buffer.setInt((B && DoNotFree) ? DoNotFreeFlag : 0);
 }
 
+const char *ContentCache::getInvalidBOM(StringRef BufStr) {
+  // If the buffer is valid, check to see if it has a UTF Byte Order Mark
+  // (BOM).  We only support UTF-8 with and without a BOM right now.  See
+  // http://en.wikipedia.org/wiki/Byte_order_mark for more information.
+  const char *InvalidBOM =
+      llvm::StringSwitch<const char *>(BufStr)
+          .StartsWith(llvm::StringLiteral::withInnerNUL("\x00\x00\xFE\xFF"),
+                      "UTF-32 (BE)")
+          .StartsWith(llvm::StringLiteral::withInnerNUL("\xFF\xFE\x00\x00"),
+                      "UTF-32 (LE)")
+          .StartsWith("\xFE\xFF", "UTF-16 (BE)")
+          .StartsWith("\xFF\xFE", "UTF-16 (LE)")
+          .StartsWith("\x2B\x2F\x76", "UTF-7")
+          .StartsWith("\xF7\x64\x4C", "UTF-1")
+          .StartsWith("\xDD\x73\x66\x73", "UTF-EBCDIC")
+          .StartsWith("\x0E\xFE\xFF", "SCSU")
+          .StartsWith("\xFB\xEE\x28", "BOCU-1")
+          .StartsWith("\x84\x31\x95\x33", "GB-18030")
+          .Default(nullptr);
+
+  return InvalidBOM;
+}
+
 const llvm::MemoryBuffer *ContentCache::getBuffer(DiagnosticsEngine &Diag,
                                                   FileManager &FM,
                                                   SourceLocation Loc,
                                                   bool *Invalid) const {
   // Lazily create the Buffer for ContentCaches that wrap files.  If we already
   // computed it, just return what we have.
   if (Buffer.getPointer() || !ContentsEntry) {
     if (Invalid)
if (getRawBuffer()->getBufferSize() != (size_t)ContentsEntry->getSize()) {
     if (Invalid) *Invalid = true;
     return Buffer.getPointer();
   }
 
   // If the buffer is valid, check to see if it has a UTF Byte Order Mark
   // (BOM).  We only support UTF-8 with and without a BOM right now.  See
   // http://en.wikipedia.org/wiki/Byte_order_mark for more information.
   StringRef BufStr = Buffer.getPointer()->getBuffer();
-  const char *InvalidBOM = llvm::StringSwitch<const char *>(BufStr)
-    .StartsWith(llvm::StringLiteral::withInnerNUL("\x00\x00\xFE\xFF"),
-                                                  "UTF-32 (BE)")
-    .StartsWith(llvm::StringLiteral::withInnerNUL("\xFF\xFE\x00\x00"),
-                                                  "UTF-32 (LE)")
-    .StartsWith("\xFE\xFF", "UTF-16 (BE)")
-    .StartsWith("\xFF\xFE", "UTF-16 (LE)")
-    .StartsWith("\x2B\x2F\x76", "UTF-7")
-    .StartsWith("\xF7\x64\x4C", "UTF-1")
-    .StartsWith("\xDD\x73\x66\x73", "UTF-EBCDIC")
-    .StartsWith("\x0E\xFE\xFF", "SCSU")
-    .StartsWith("\xFB\xEE\x28", "BOCU-1")
-    .StartsWith("\x84\x31\x95\x33", "GB-18030")
-    .Default(nullptr);
+  const char *InvalidBOM = getInvalidBOM(BufStr);
 
   if (InvalidBOM) {
     Diag.Report(Loc, diag::err_unsupported_bom)
       << InvalidBOM << ContentsEntry->getName();
     Buffer.setInt(Buffer.getInt() | InvalidFlag);
   }
 
   if (Invalid)

clang/tools/clang-format/ClangFormat.cpp

for (const auto &R : Replaces) {
     outs() << "<replacement "
            << "offset='" << R.getOffset() << "' "
            << "length='" << R.getLength() << "'>";
     outputReplacementXML(R.getReplacementText());
     outs() << "</replacement>\n";
   }
 }
 
-// If BufStr has an invalid BOM, returns the BOM name; otherwise, returns
-// nullptr.
-static const char *getInValidBOM(StringRef BufStr) {
-  // Check to see if the buffer has a UTF Byte Order Mark (BOM).
-  // We only support UTF-8 with and without a BOM right now.  See
-  // https://en.wikipedia.org/wiki/Byte_order_mark#Byte_order_marks_by_encoding
-  // for more information.
-  const char *InvalidBOM =
-      llvm::StringSwitch<const char *>(BufStr)
-          .StartsWith(llvm::StringLiteral::withInnerNUL("\x00\x00\xFE\xFF"),
-                      "UTF-32 (BE)")
-          .StartsWith(llvm::StringLiteral::withInnerNUL("\xFF\xFE\x00\x00"),
-                      "UTF-32 (LE)")
-          .StartsWith("\xFE\xFF", "UTF-16 (BE)")
-          .StartsWith("\xFF\xFE", "UTF-16 (LE)")
-          .StartsWith("\x2B\x2F\x76", "UTF-7")
-          .StartsWith("\xF7\x64\x4C", "UTF-1")
-          .StartsWith("\xDD\x73\x66\x73", "UTF-EBCDIC")
-          .StartsWith("\x0E\xFE\xFF", "SCSU")
-          .StartsWith("\xFB\xEE\x28", "BOCU-1")
-          .StartsWith("\x84\x31\x95\x33", "GB-18030")
-          .Default(nullptr);
-  return InvalidBOM;
-}
-
 static bool
 emitReplacementWarnings(const Replacements &Replaces, StringRef AssumedFileName,
                         const std::unique_ptr<llvm::MemoryBuffer> &Code) {
   if (Replaces.empty()) {
     return false;
   }
 
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
if (std::error_code EC = CodeOrErr.getError()) {
     return true;
   }
   std::unique_ptr<llvm::MemoryBuffer> Code = std::move(CodeOrErr.get());
   if (Code->getBufferSize() == 0)
     return false; // Empty files are formatted correctly.
 
   StringRef BufStr = Code->getBuffer();
 
-  const char *InvalidBOM = getInValidBOM(BufStr);
+  const char *InvalidBOM = SrcMgr::ContentCache::getInvalidBOM(BufStr);
 
   if (InvalidBOM) {
     errs() << "error: encoding with unsupported byte order mark \""
            << InvalidBOM << "\" detected";
     if (FileName != "-")
       errs() << " in file '" << FileName << "'";
     errs() << ".\n";
     return true;

clang/unittests/Basic/SourceManagerTest.cpp

TEST_F(SourceManagerTest, locationPrintTest) {
   EXPECT_EQ(SourceRange(BeginLoc, BeginEOLLoc).printToString(SourceMgr),
             "</mainFile.cpp:1:1, col:7>");
   EXPECT_EQ(SourceRange(BeginLoc, EndLoc).printToString(SourceMgr),
             "</mainFile.cpp:1:1, line:4:7>");
   EXPECT_EQ(SourceRange(BeginLoc, HeaderLoc).printToString(SourceMgr),
             "</mainFile.cpp:1:1, /test-header.h:1:1>");
 }
 
+TEST_F(SourceManagerTest, getInvalidBOM) {
+  ASSERT_EQ(SrcMgr::ContentCache::getInvalidBOM(""), nullptr);
+  ASSERT_EQ(SrcMgr::ContentCache::getInvalidBOM("\x00\x00\x00"), nullptr);
+  ASSERT_EQ(SrcMgr::ContentCache::getInvalidBOM("\xFF\xFF\xFF"), nullptr);
+  ASSERT_EQ(SrcMgr::ContentCache::getInvalidBOM("#include <iostream>"),
+            nullptr);
+
+  ASSERT_EQ(StringRef(SrcMgr::ContentCache::getInvalidBOM(
+                "\xFE\xFF#include <iostream>")),
+            "UTF-16 (BE)");
+  ASSERT_EQ(StringRef(SrcMgr::ContentCache::getInvalidBOM(
+                "\xFF\xFE#include <iostream>")),
+            "UTF-16 (LE)");
+  ASSERT_EQ(StringRef(SrcMgr::ContentCache::getInvalidBOM(
+                "\x2B\x2F\x76#include <iostream>")),
+            "UTF-7");
+  ASSERT_EQ(StringRef(SrcMgr::ContentCache::getInvalidBOM(
+                "\xF7\x64\x4C#include <iostream>")),
+            "UTF-1");
+  ASSERT_EQ(StringRef(SrcMgr::ContentCache::getInvalidBOM(
+                "\xDD\x73\x66\x73#include <iostream>")),
+            "UTF-EBCDIC");
+  ASSERT_EQ(StringRef(SrcMgr::ContentCache::getInvalidBOM(
+                "\x0E\xFE\xFF#include <iostream>")),
+            "SCSU");
+  ASSERT_EQ(StringRef(SrcMgr::ContentCache::getInvalidBOM(
+                "\xFB\xEE\x28#include <iostream>")),
+            "BOCU-1");
+  ASSERT_EQ(StringRef(SrcMgr::ContentCache::getInvalidBOM(
+                "\x84\x31\x95\x33#include <iostream>")),
+            "GB-18030");
+  ASSERT_EQ(StringRef(SrcMgr::ContentCache::getInvalidBOM(
+                llvm::StringLiteral::withInnerNUL(
+                    "\x00\x00\xFE\xFF#include <iostream>"))),
+            "UTF-32 (BE)");
+  ASSERT_EQ(StringRef(SrcMgr::ContentCache::getInvalidBOM(
+                llvm::StringLiteral::withInnerNUL(
+                    "\xFF\xFE\x00\x00#include <iostream>"))),
+            "UTF-32 (LE)");
+}
+
 #if defined(LLVM_ON_UNIX)
 
 TEST_F(SourceManagerTest, getMacroArgExpandedLocation) {
   const char *header =
     "#define FM(x,y) x\n";
 
   const char *main =
     "#include \"/test-header.h\"\n"