[clang-format][NFC] Clean up class HeaderIncludes and Format.cpp

[MyDeveloperDay]

clang/lib/Format/Format.cpp

2774

Did I miss where this comes from now?

[kwk]

I didn't see much difference in what this patch does compare to mine but I saw that it removes the need for instantiating multiple llvm::Regex objects from a single static pattern. But that's something I've just done in a new revision of my own patch: https://reviews.llvm.org/D134733?vs=463205&id=463786#toc . This fast clean up makes it only harder for me to get my patch in but essentially both do the same thing except that I also have a single convenience function for trimming include names from " and <>". Also I have removed the need to duplicate the knowledge of which matching group one has to pick in order to get to the include name. This knowledge is now local to a function.

clang/lib/Format/Format.cpp

2774

@MyDeveloperDay clang/lib/Tooling/Inclusions/HeaderIncludes.cpp still has this:

const char IncludeRegexPattern[] =
    R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";

And in clang/lib/Tooling/Inclusions/HeaderIncludes.cpp @owenpan uses it to initialize the final regex

const llvm::Regex HeaderIncludes::IncludeRegex(IncludeRegexPattern);

The fact that we have two regex that are identical is an issue on its own that I tried to address with my patch as well. I didn't initialize the regex like @owenpan does here but I had a function to return it. Eventually a function makes it easier to apply the injection from a config file as you've suggested here. So I favor my solution.

[owenpan]

@kwk D134733 reminded me of https://reviews.llvm.org/D121370#3401173, which I realized didn't go far enough. This NFC patch is what I should have suggested to you then. If you want to do the cleanup differently, you can either make suggestions here or create another NFC patch.

clang/lib/Format/Format.cpp

2774

The fact that we have two regex that are identical is an issue on its own that I tried to address with my patch as well.

It should be addressed in a separate NFC patch such as this one.

I didn't initialize the regex like @owenpan does here but I had a function to return it. Eventually a function makes it easier to apply the injection from a config file as you've suggested here. So I favor my solution.

Making IncludeRegex a public static const member is one of the better solutions when IncludeRegexPattern is fixed as it has been. If and when we decide to support user specified patterns, we will make any necessary changes then.

[kwk]

clang/lib/Format/Format.cpp

2774

The fact that we have two regex that are identical is an issue on its own that I tried to address with my patch as well.

It should be addressed in a separate NFC patch such as this one.

Ehm, I thought I *did* create a separate NFC patch with D134733. I prefixed it with [chore] but that is as good as NFC. I can rename it if you want.

I didn't initialize the regex like @owenpan does here but I had a function to return it. Eventually a function makes it easier to apply the injection from a config file as you've suggested here. So I favor my solution.

Making IncludeRegex a public static const member is one of the better solutions when IncludeRegexPattern is fixed as it has been. If and when we decide to support user specified patterns, we will make any necessary changes then.

Okay, but you could have suggested that in D134733, no? I've made the change in D134733 here: https://reviews.llvm.org/D134733?vs=463205&id=464196#toc, so the regex is static const. But I've also outsourced the function for accessing the include name so the logic is at one place not scattered over and over and the trimming is also in its own function. Having everything going through that functions is easier for maintenance IMHO. Before I wondered why we had two include regex patterns (both the same btw.) and why an include name wasn't found when I had changed the Matches[2] to Matches[3] for example. That won't happen when its going through the function. You just change it in one place and not "plenty".

I hope we can agree that my change is now complete with additions from yours here. I most certainly don't want to disrupt your workflow and I apologize if I had. Unfortunately text can be misinterpreted way too much.

[owenpan]

clang/lib/Format/Format.cpp

2774

Ehm, I thought I *did* create a separate NFC patch with D134733. I prefixed it with [chore] but that is as good as NFC. I can rename it if you want.

I didn't know by chore you meant NFC. Thanks for renaming it.

Okay, but you could have suggested that in D134733, no?

It would be too many places to point out as you can tell from this patch. When mixed with other unrelated changes in D134733, it would be more difficult for me to review and other people to follow. Making this patch allows me to focus on the best way to solve the problem without worrying about any additional "noise".

I hope we can agree that my change is now complete with additions from yours here.

It seems that you have applied this patch in its entirety to D134733. Why not just wait for it to land and rebase D134733 afterward?

I most certainly don't want to disrupt your workflow and I apologize if I had.

Not at all. If anything, it's probably I who have disrupted your workflow. :)

[kwk]

clang/lib/Format/Format.cpp

2774

Ehm, I thought I *did* create a separate NFC patch with D134733. I prefixed it with [chore] but that is as good as NFC. I can rename it if you want.

I didn't know by chore you meant NFC. Thanks for renaming it.

Your welcome.

Okay, but you could have suggested that in D134733, no?

It would be too many places to point out as you can tell from this patch. When mixed with other unrelated changes in D134733, it would be more difficult for me to review and other people to follow. Making this patch allows me to focus on the best way to solve the problem without worrying about any additional "noise".

I get that but I thought that my NFC patch was doing exactly that:

1. Make it one not two regexes
2. Make the regex a static member and not call a function to get it.

I hope we can agree that my change is now complete with additions from yours here.

It seems that you have applied this patch in its entirety to D134733. Why not just wait for it to land and rebase D134733 afterward?

I feel that I introduced this cleanup myself by having only one regex and not two. The only difference from mine to your patch was that you used a static member and I made a function call and didn't avoid the instantiations of regex objects.

I most certainly don't want to disrupt your workflow and I apologize if I had.

Not at all. If anything, it's probably I who have disrupted your workflow. :)

This seems just weird afterall. So why don't you land your patch and I submit mine?

clang/include/clang/Tooling/Inclusions/HeaderIncludes.h

llvm::Optional<tooling::Replacement> insert(llvm::StringRef Header,
                                               bool IsAngled) const;
 
   /// Removes all existing #includes of \p Header quoted with <> if \p IsAngled
   /// is true or "" if \p IsAngled is false.
   /// This doesn't resolve the header file path; it only deletes #includes with
   /// exactly the same spelling.
   tooling::Replacements remove(llvm::StringRef Header, bool IsAngled) const;
 
+  // Matches a whole #include directive.
+  static const llvm::Regex IncludeRegex;
+
 private:
   struct Include {
     Include(StringRef Name, tooling::Range R) : Name(Name), R(R) {}
 
     // An include header quoted with either <> or "".
     std::string Name;
     // The range of the whole line of include directive including any leading
     // whitespaces and trailing comment.
private:
   // declaration).
   unsigned MaxInsertOffset;
   IncludeCategoryManager Categories;
   // Record the offset of the end of the last include in each category.
   std::unordered_map<int, int> CategoryEndOffsets;
 
   // All possible priorities.
   std::set<int> Priorities;
-
-  // Matches a whole #include directive.
-  llvm::Regex IncludeRegex;
 };
 
 } // namespace tooling
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLING_INCLUSIONS_HEADERINCLUDES_H

clang/lib/Format/Format.cpp

static void sortCppIncludes(const FormatStyle &Style,
   // FIXME: better error handling. For now, just skip the replacement for the
   // release version.
   if (Err) {
     llvm::errs() << llvm::toString(std::move(Err)) << "\n";
     assert(false);
   }
 }
 
-namespace {
-
-const char CppIncludeRegexPattern[] =
-    R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
-
-} // anonymous namespace
-
 tooling::Replacements sortCppIncludes(const FormatStyle &Style, StringRef Code,
                                       ArrayRef<tooling::Range> Ranges,
                                       StringRef FileName,
                                       tooling::Replacements &Replaces,
                                       unsigned *Cursor) {
   unsigned Prev = llvm::StringSwitch<size_t>(Code)
                       .StartsWith("\xEF\xBB\xBF", 3) // UTF-8 BOM
                       .Default(0);
   unsigned SearchFrom = 0;
-  llvm::Regex IncludeRegex(CppIncludeRegexPattern);
   SmallVector<StringRef, 4> Matches;
   SmallVector<IncludeDirective, 16> IncludesInBlock;
 
   // In compiled files, consider the first #include to be the main #include of
   // the file if it is not a system #include. This ensures that the header
   // doesn't have hidden dependencies
   // (http://llvm.org/docs/CodingStandards.html#include-style).
   //
for (;;) {
     const bool EmptyLineSkipped =
         Trimmed.empty() &&
         (Style.IncludeStyle.IncludeBlocks == tooling::IncludeStyle::IBS_Merge ||
          Style.IncludeStyle.IncludeBlocks ==
              tooling::IncludeStyle::IBS_Regroup);
 
     bool MergeWithNextLine = Trimmed.endswith("\\");
     if (!FormattingOff && !MergeWithNextLine) {
-      if (IncludeRegex.match(Line, &Matches)) {
+      if (tooling::HeaderIncludes::IncludeRegex.match(Line, &Matches)) {
         StringRef IncludeName = Matches[2];
         if (Line.contains("/*") && !Line.contains("*/")) {
           // #include with a start of a block comment, but without the end.
           // Need to keep all the lines until the end of the comment together.
           // FIXME: This is somehow simplified check that probably does not work
           // correctly if there are multiple comments on a line.
           Pos = Code.find("*/", SearchFrom);
           Line = Code.substr(
formatReplacements(StringRef Code, const tooling::Replacements &Replaces,
   };
   return processReplacements(Reformat, Code, *SortedReplaces, Style);
 }
 
 namespace {
 
 inline bool isHeaderInsertion(const tooling::Replacement &Replace) {
   return Replace.getOffset() == UINT_MAX && Replace.getLength() == 0 &&
-         llvm::Regex(CppIncludeRegexPattern)
-             .match(Replace.getReplacementText());
+         tooling::HeaderIncludes::IncludeRegex.match(
+             Replace.getReplacementText());
 }
 
 inline bool isHeaderDeletion(const tooling::Replacement &Replace) {
   return Replace.getOffset() == UINT_MAX && Replace.getLength() == 1;
 }
 
 // FIXME: insert empty lines between newly created blocks.
 tooling::Replacements
for (const auto &R : Replaces) {
         // Ignore the deletion on conflict.
         llvm::errs() << "Failed to add header deletion replacement for "
                      << Header << ": " << llvm::toString(std::move(Err))
                      << "\n";
       }
     }
   }
 
-  llvm::Regex IncludeRegex = llvm::Regex(CppIncludeRegexPattern);
   llvm::SmallVector<StringRef, 4> Matches;
   for (const auto &R : HeaderInsertions) {
     auto IncludeDirective = R.getReplacementText();
-    bool Matched = IncludeRegex.match(IncludeDirective, &Matches);
+    bool Matched =
+        tooling::HeaderIncludes::IncludeRegex.match(IncludeDirective, &Matches);
     assert(Matched && "Header insertion replacement must have replacement text "
                       "'#include ...'");
     (void)Matched;
     auto IncludeName = Matches[2];
     auto Replace =
         Includes.insert(IncludeName.trim("\"<>"), IncludeName.startswith("<"));
     if (Replace) {
       auto Err = Result.add(*Replace);

clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

if (!Matching.empty()) {
     llvm::Regex MainIncludeRegex(HeaderStem.str() + Style.IncludeIsMainRegex,
                                  llvm::Regex::IgnoreCase);
     if (MainIncludeRegex.match(Matching))
       return true;
   }
   return false;
 }
 
+const llvm::Regex HeaderIncludes::IncludeRegex(IncludeRegexPattern);
+
 HeaderIncludes::HeaderIncludes(StringRef FileName, StringRef Code,
                                const IncludeStyle &Style)
     : FileName(FileName), Code(Code), FirstIncludeOffset(-1),
       MinInsertOffset(
           getOffsetAfterHeaderGuardsAndComments(FileName, Code, Style)),
       MaxInsertOffset(MinInsertOffset +
                       getMaxHeaderInsertionOffset(
                           FileName, Code.drop_front(MinInsertOffset), Style)),
-      Categories(Style, FileName),
-      IncludeRegex(llvm::Regex(IncludeRegexPattern)) {
+      Categories(Style, FileName) {
   // Add 0 for main header and INT_MAX for headers that are not in any
   // category.
   Priorities = {0, INT_MAX};
   for (const auto &Category : Style.IncludeCategories)
     Priorities.insert(Category.Priority);
   SmallVector<StringRef, 32> Lines;
   Code.drop_front(MinInsertOffset).split(Lines, "\n");