[include-cleaner] Merge 2 parseIWYUPragma impls in libToolingInclusions

[sammccall]

Summary

Based on include-cleaner's version, but:

• remove assert that can fail for input /\<newline>* */
• assert was also checking the wrong condition: that the prefix *differed* from either // or from /*. Avoid use of strncmp where we can.
• add a comment that the brittleness of the text matching is intentional

[hokein]

Thanks! The change is LG, I think it would be great to have some tests.

clang-tools-extra/include-cleaner/lib/Record.cpp

143–144

I'd like to have some unittests for this function (the cases you mentioned are really good, I don't want to lose them), but this function is not exposed. Maybe expose it or even address the FIXME?

[sammccall]

Move to libTooling, remove clangd impl, add tests

[sammccall]

clean dead tests

[sammccall]

clang-tools-extra/include-cleaner/lib/Record.cpp

143–144

Done.

This stings a little as I'd recently done this in 5d2d527c32, after which D136071 copied the impl without any tests and added new bugs.

[sammccall]

tweak whitespace and */ behavior

[hokein]

Thanks!

[ABataev]

Looks like it breaks the build:

llvm-project/clang-tools-extra/clangd/index/CanonicalIncludes.cpp: In member function ‘virtual bool clang::clangd::collectIWYUHeaderMaps(clang::clangd::CanonicalIncludes*)::PragmaCommentHandler::HandleComment(clang::Preprocessor&, clang::SourceRange)’:
llvm-project/clang-tools-extra/clangd/index/CanonicalIncludes.cpp:713:21: error: ‘parseIWYUPragma’ was not declared in this scope
       auto Pragma = parseIWYUPragma(
                     ^~~~~~~~~~~~~~~

[sammccall]

In D138780#3953229, @ABataev wrote:

Looks like it breaks the build:

llvm-project/clang-tools-extra/clangd/index/CanonicalIncludes.cpp: In member function ‘virtual bool clang::clangd::collectIWYUHeaderMaps(clang::clangd::CanonicalIncludes*)::PragmaCommentHandler::HandleComment(clang::Preprocessor&, clang::SourceRange)’:
llvm-project/clang-tools-extra/clangd/index/CanonicalIncludes.cpp:713:21: error: ‘parseIWYUPragma’ was not declared in this scope
       auto Pragma = parseIWYUPragma(
                     ^~~~~~~~~~~~~~~

Sorry, this is fixed in 551c7e81891abcf49aff625187f348ff119c4618 (I was running tests in the wrong terminal...)

clang-tools-extra/clangd/Headers.h

 #include <string>
 
 namespace clang {
 namespace clangd {
 
 /// Returns true if \p Include is literal include like "path" or <path>.
 bool isLiteralInclude(llvm::StringRef Include);
 
-/// If Text begins an Include-What-You-Use directive, returns it.
-/// Given "// IWYU pragma: keep", returns "keep".
-/// Input is a null-terminated char* as provided by SM.getCharacterData().
-/// (This should not be StringRef as we do *not* want to scan for its length).
-llvm::Optional<StringRef> parseIWYUPragma(const char *Text);
-
 /// Represents a header file to be #include'd.
 struct HeaderFile {
   std::string File;
   /// If this is true, `File` is a literal string quoted with <> or "" that
   /// can be #included directly; otherwise, `File` is an absolute file path.
   bool Verbatim;
 
   bool valid() const;

clang-tools-extra/clangd/Headers.cpp

 #include "clang/Tooling/Inclusions/HeaderAnalysis.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Path.h"
 #include <cstring>
 
 namespace clang {
 namespace clangd {
 
-llvm::Optional<StringRef> parseIWYUPragma(const char *Text) {
-  // This gets called for every comment seen in the preamble, so it's quite hot.
-  constexpr llvm::StringLiteral IWYUPragma = "// IWYU pragma: ";
-  if (strncmp(Text, IWYUPragma.data(), IWYUPragma.size()))
-    return llvm::None;
-  Text += IWYUPragma.size();
-  const char *End = Text;
-  while (*End != 0 && *End != '\n')
-    ++End;
-  return StringRef(Text, End - Text);
-}
-
 class IncludeStructure::RecordHeaders : public PPCallbacks,
                                         public CommentHandler {
 public:
   RecordHeaders(const CompilerInstance &CI, IncludeStructure *Out)
       : SM(CI.getSourceManager()),
         HeaderInfo(CI.getPreprocessor().getHeaderSearchInfo()), Out(Out) {}
 
   // Record existing #includes - both written and resolved paths. Only #includes
void FileChanged(SourceLocation Loc, FileChangeReason Reason,
     }
     case PPCallbacks::RenameFile:
     case PPCallbacks::SystemHeaderPragma:
       break;
     }
   }
 
   bool HandleComment(Preprocessor &PP, SourceRange Range) override {
-    auto Pragma = parseIWYUPragma(SM.getCharacterData(Range.getBegin()));
+    auto Pragma =
+        tooling::parseIWYUPragma(SM.getCharacterData(Range.getBegin()));
     if (!Pragma)
       return false;
 
     if (inMainFile()) {
       // Given:
       //
       // #include "foo.h"
       // #include "bar.h" // IWYU pragma: keep

clang-tools-extra/clangd/unittests/HeadersTests.cpp

 )cpp";
 
   auto Includes = collectIncludes();
   EXPECT_TRUE(Includes.hasIWYUExport(getID("export.h", Includes)));
   EXPECT_TRUE(Includes.hasIWYUExport(getID("begin_exports.h", Includes)));
   EXPECT_FALSE(Includes.hasIWYUExport(getID("none.h", Includes)));
 }
 
-TEST(Headers, ParseIWYUPragma) {
-  EXPECT_THAT(parseIWYUPragma("// IWYU pragma: keep"), HasValue(Eq("keep")));
-  EXPECT_THAT(parseIWYUPragma("// IWYU pragma: keep\netc"),
-              HasValue(Eq("keep")));
-  EXPECT_EQ(parseIWYUPragma("/* IWYU pragma: keep"), llvm::None)
-      << "Only // comments supported!";
-  EXPECT_EQ(parseIWYUPragma("//  IWYU pragma: keep"), llvm::None)
-      << "Sensitive to whitespace";
-  EXPECT_EQ(parseIWYUPragma("// IWYU pragma:keep"), llvm::None)
-      << "Sensitive to whitespace";
-}
-
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/include-cleaner/lib/Record.cpp

private:
 
   bool Active = false;
   RecordedPP &Recorded;
   const Preprocessor &PP;
   const SourceManager &SM;
 };
 
 } // namespace
 
-// FIXME: this is a mirror of clang::clangd::parseIWYUPragma, move to libTooling
-// to share the code?
-static llvm::Optional<StringRef> parseIWYUPragma(const char *Text) {
-  assert(strncmp(Text, "//", 2) || strncmp(Text, "/*", 2));
-  constexpr llvm::StringLiteral IWYUPragma = " IWYU pragma: ";
-  Text += 2; // Skip the comment start, // or /*.
-  if (strncmp(Text, IWYUPragma.data(), IWYUPragma.size()))
-    return llvm::None;
-  Text += IWYUPragma.size();
-  const char *End = Text;
-  while (*End != 0 && *End != '\n')
-    ++End;
-  return StringRef(Text, End - Text);
-}
-
 class PragmaIncludes::RecordPragma : public PPCallbacks, public CommentHandler {
 public:
   RecordPragma(const CompilerInstance &CI, PragmaIncludes *Out)
       : SM(CI.getSourceManager()),
         HeaderInfo(CI.getPreprocessor().getHeaderSearchInfo()), Out(Out),
         UniqueStrings(Arena) {}
 
   void FileChanged(SourceLocation Loc, FileChangeReason Reason,
                    SrcMgr::CharacteristicKind FileType,
if ((Top.Block && HashLine > Top.SeenAtLine) ||
         Out->ShouldKeep.insert(HashLine);
     }
     if (!Top.Block) // Pop immediately for single-line export pragma.
       ExportStack.pop_back();
   }
 
   bool HandleComment(Preprocessor &PP, SourceRange Range) override {
     auto &SM = PP.getSourceManager();
-    auto Pragma = parseIWYUPragma(SM.getCharacterData(Range.getBegin()));
+    auto Pragma =
+        tooling::parseIWYUPragma(SM.getCharacterData(Range.getBegin()));
     if (!Pragma)
       return false;
 
     if (Pragma->consume_front("private")) {
       auto *FE = SM.getFileEntryForID(SM.getFileID(Range.getBegin()));
       if (!FE)
         return false;
       StringRef PublicHeader;

clang/include/clang/Tooling/Inclusions/HeaderAnalysis.h

 //===--- HeaderAnalysis.h -----------------------------------------*-C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLING_INCLUSIONS_HEADER_ANALYSIS_H
 #define LLVM_CLANG_TOOLING_INCLUSIONS_HEADER_ANALYSIS_H
 
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
 namespace clang {
 class FileEntry;
 class SourceManager;
 class HeaderSearch;
 
 namespace tooling {
 
 /// Returns true if the given physical file is a self-contained header.
 ///
 /// A header is considered self-contained if
 //   - it has a proper header guard or has been #imported
 //   - *and* it doesn't have a dont-include-me pattern.
 ///
 /// This function can be expensive as it may scan the source code to find out
 /// dont-include-me pattern heuristically.
 bool isSelfContainedHeader(const FileEntry *FE, const SourceManager &SM,
                            HeaderSearch &HeaderInfo);
 
+/// If Text begins an Include-What-You-Use directive, returns it.
+/// Given "// IWYU pragma: keep", returns "keep".
+/// Input is a null-terminated char* as provided by SM.getCharacterData().
+/// (This should not be StringRef as we do *not* want to scan for its length).
+/// For multi-line comments, we return only the first line.
+llvm::Optional<llvm::StringRef> parseIWYUPragma(const char *Text);
+
 } // namespace tooling
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLING_INCLUSIONS_HEADER_ANALYSIS_H

clang/lib/Tooling/Inclusions/HeaderAnalysis.cpp

if (!HeaderInfo.isFileMultipleIncludeGuarded(FE) &&
       !HeaderInfo.hasFileBeenImported(FE))
     return false;
   // This pattern indicates that a header can't be used without
   // particular preprocessor state, usually set up by another header.
   return !isDontIncludeMeHeader(
       const_cast<SourceManager &>(SM).getMemoryBufferForFileOrNone(FE).value_or(
           llvm::MemoryBufferRef()));
 }
+
+llvm::Optional<StringRef> parseIWYUPragma(const char *Text) {
+  // Skip the comment start, // or /*.
+  if (Text[0] != '/' || (Text[1] != '/' && Text[1] != '*'))
+    return llvm::None;
+  bool BlockComment = Text[1] == '*';
+  Text += 2;
+
+  // Per spec, direcitves are whitespace- and case-sensitive.
+  constexpr llvm::StringLiteral IWYUPragma = " IWYU pragma: ";
+  if (strncmp(Text, IWYUPragma.data(), IWYUPragma.size()))
+    return llvm::None;
+  Text += IWYUPragma.size();
+  const char *End = Text;
+  while (*End != 0 && *End != '\n')
+    ++End;
+  StringRef Rest(Text, End - Text);
+  // Strip off whitespace and comment markers to avoid confusion. This isn't
+  // fully-compatible with IWYU, which splits into whitespace-delimited tokens.
+  if (BlockComment)
+    Rest.consume_back("*/");
+  return Rest.trim();
+}
+
 } // namespace clang::tooling

clang/unittests/Tooling/HeaderAnalysisTest.cpp

 //===- unittest/Tooling/HeaderAnalysisTest.cpp ----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/Inclusions/HeaderAnalysis.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Testing/TestAST.h"
+#include "llvm/Testing/Support/SupportHelpers.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tooling {
 namespace {
+using llvm::ValueIs;
+using testing::Eq;
 
 TEST(HeaderAnalysisTest, IsSelfContained) {
   TestInputs Inputs;
   Inputs.Code = R"cpp(
   #include "headerguard.h"
   #include "pragmaonce.h"
   #import "imported.h"
 
TEST(HeaderAnalysisTest, IsSelfContained) {
   EXPECT_TRUE(isSelfContainedHeader(FM.getFile("headerguard.h").get(), SM, HI));
   EXPECT_TRUE(isSelfContainedHeader(FM.getFile("pragmaonce.h").get(), SM, HI));
   EXPECT_TRUE(isSelfContainedHeader(FM.getFile("imported.h").get(), SM, HI));
 
   EXPECT_FALSE(isSelfContainedHeader(FM.getFile("unguarded.h").get(), SM, HI));
   EXPECT_FALSE(isSelfContainedHeader(FM.getFile("bad.h").get(), SM, HI));
 }
 
+TEST(HeaderAnalysisTest, ParseIWYUPragma) {
+  EXPECT_THAT(parseIWYUPragma("// IWYU pragma: keep"), ValueIs(Eq("keep")));
+  EXPECT_THAT(parseIWYUPragma("// IWYU pragma:   keep  me\netc"),
+              ValueIs(Eq("keep  me")));
+  EXPECT_THAT(parseIWYUPragma("/* IWYU pragma: keep */"), ValueIs(Eq("keep")));
+  EXPECT_EQ(parseIWYUPragma("//  IWYU pragma: keep"), llvm::None)
+      << "Prefix is sensitive to whitespace";
+  EXPECT_EQ(parseIWYUPragma("// IWYU pragma:keep"), llvm::None)
+      << "Prefix is sensitive to whitespace";
+  EXPECT_EQ(parseIWYUPragma("/\n* IWYU pragma: keep */"), llvm::None)
+      << "Must start with /* or //";
+}
+
 } // namespace
 } // namespace tooling
 } // namespace clang