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

[Owen Pan via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7cbc9206697e: [clang-format][NFC] Clean up class 
HeaderIncludes and Format.cpp (authored by owenpan).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134852/new/

https://reviews.llvm.org/D134852

Files:
  clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
  clang/lib/Format/Format.cpp
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -266,6 +266,8 @@
   return false;
 }
 
+const llvm::Regex HeaderIncludes::IncludeRegex(IncludeRegexPattern);
+
 HeaderIncludes::HeaderIncludes(StringRef FileName, StringRef Code,
const IncludeStyle )
 : FileName(FileName), Code(Code), FirstIncludeOffset(-1),
@@ -274,8 +276,7 @@
   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};
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -2769,13 +2769,6 @@
   }
 }
 
-namespace {
-
-const char CppIncludeRegexPattern[] =
-R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
-
-} // anonymous namespace
-
 tooling::Replacements sortCppIncludes(const FormatStyle , StringRef Code,
   ArrayRef Ranges,
   StringRef FileName,
@@ -2785,7 +2778,6 @@
   .StartsWith("\xEF\xBB\xBF", 3) // UTF-8 BOM
   .Default(0);
   unsigned SearchFrom = 0;
-  llvm::Regex IncludeRegex(CppIncludeRegexPattern);
   SmallVector Matches;
   SmallVector IncludesInBlock;
 
@@ -2842,7 +2834,7 @@
 
 bool MergeWithNextLine = Trimmed.endswith("\\");
 if (!FormattingOff && !MergeWithNextLine) {
-  if (IncludeRegex.match(Line, )) {
+  if (tooling::HeaderIncludes::IncludeRegex.match(Line, )) {
 StringRef IncludeName = Matches[2];
 if (Line.contains("/*") && !Line.contains("*/")) {
   // #include with a start of a block comment, but without the end.
@@ -3120,8 +3112,8 @@
 
 inline bool isHeaderInsertion(const tooling::Replacement ) {
   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 ) {
@@ -3173,11 +3165,11 @@
 }
   }
 
-  llvm::Regex IncludeRegex = llvm::Regex(CppIncludeRegexPattern);
   llvm::SmallVector Matches;
   for (const auto  : HeaderInsertions) {
 auto IncludeDirective = R.getReplacementText();
-bool Matched = IncludeRegex.match(IncludeDirective, );
+bool Matched =
+tooling::HeaderIncludes::IncludeRegex.match(IncludeDirective, );
 assert(Matched && "Header insertion replacement must have replacement text "
   "'#include ...'");
 (void)Matched;
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -79,6 +79,9 @@
   /// 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) {}
@@ -124,9 +127,6 @@
 
   // All possible priorities.
   std::set Priorities;
-
-  // Matches a whole #include directive.
-  llvm::Regex IncludeRegex;
 };
 
 } // namespace tooling
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Konrad Wilhelm Kleine via Phabricator via cfe-commits]

kwk accepted this revision.
kwk added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Format/Format.cpp:2774
-
-const char CppIncludeRegexPattern[] =
-R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";

owenpan wrote:
> kwk wrote:
> > owenpan wrote:
> > > kwk wrote:
> > > > MyDeveloperDay wrote:
> > > > > Did I miss where this comes from now?
> > > > @MyDeveloperDay `clang/lib/Tooling/Inclusions/HeaderIncludes.cpp` still 
> > > > has this:
> > > > 
> > > > ```lang=c++
> > > > const char IncludeRegexPattern[] =
> > > > R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
> > > > ```
> > > > 
> > > > And in `clang/lib/Tooling/Inclusions/HeaderIncludes.cpp` @owenpan uses 
> > > > it to initialize the final regex 
> > > > 
> > > > ```lang=c++
> > > > 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](https://reviews.llvm.org/D134733) 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](https://reviews.llvm.org/D134733#3821957). So I favor my 
> > > > solution.
> > > > 
> > > > 
> > > > 
> > > > 
> > > > The fact that we have two regex that are identical is an issue on its 
> > > > own that I tried to address with [my 
> > > > patch](https://reviews.llvm.org/D134733) 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](https://reviews.llvm.org/D134733#3821957). 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.
> > > > The fact that we have two regex that are identical is an issue on its 
> > > > own that I tried to address with [my 
> > > > patch](https://reviews.llvm.org/D134733) 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](https://reviews.llvm.org/D134733#3821957). 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=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.
> > 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 

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/lib/Format/Format.cpp:2774
-
-const char CppIncludeRegexPattern[] =
-R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";

kwk wrote:
> owenpan wrote:
> > kwk wrote:
> > > MyDeveloperDay wrote:
> > > > Did I miss where this comes from now?
> > > @MyDeveloperDay `clang/lib/Tooling/Inclusions/HeaderIncludes.cpp` still 
> > > has this:
> > > 
> > > ```lang=c++
> > > const char IncludeRegexPattern[] =
> > > R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
> > > ```
> > > 
> > > And in `clang/lib/Tooling/Inclusions/HeaderIncludes.cpp` @owenpan uses it 
> > > to initialize the final regex 
> > > 
> > > ```lang=c++
> > > 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](https://reviews.llvm.org/D134733) 
> > > 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](https://reviews.llvm.org/D134733#3821957). So I favor my solution.
> > > 
> > > 
> > > 
> > > 
> > > The fact that we have two regex that are identical is an issue on its own 
> > > that I tried to address with [my patch](https://reviews.llvm.org/D134733) 
> > > 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](https://reviews.llvm.org/D134733#3821957). 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.
> > > The fact that we have two regex that are identical is an issue on its own 
> > > that I tried to address with [my patch](https://reviews.llvm.org/D134733) 
> > > 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](https://reviews.llvm.org/D134733#3821957). 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=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.
> 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 

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

[Konrad Wilhelm Kleine via Phabricator via cfe-commits]

kwk added inline comments.



Comment at: clang/lib/Format/Format.cpp:2774
-
-const char CppIncludeRegexPattern[] =
-R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";

owenpan wrote:
> kwk wrote:
> > MyDeveloperDay wrote:
> > > Did I miss where this comes from now?
> > @MyDeveloperDay `clang/lib/Tooling/Inclusions/HeaderIncludes.cpp` still has 
> > this:
> > 
> > ```lang=c++
> > const char IncludeRegexPattern[] =
> > R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
> > ```
> > 
> > And in `clang/lib/Tooling/Inclusions/HeaderIncludes.cpp` @owenpan uses it 
> > to initialize the final regex 
> > 
> > ```lang=c++
> > 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](https://reviews.llvm.org/D134733) 
> > 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](https://reviews.llvm.org/D134733#3821957). So I favor my solution.
> > 
> > 
> > 
> > 
> > The fact that we have two regex that are identical is an issue on its own 
> > that I tried to address with [my patch](https://reviews.llvm.org/D134733) 
> > 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](https://reviews.llvm.org/D134733#3821957). 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.
> > The fact that we have two regex that are identical is an issue on its own 
> > that I tried to address with [my patch](https://reviews.llvm.org/D134733) 
> > 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](https://reviews.llvm.org/D134733#3821957). 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=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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134852/new/

https://reviews.llvm.org/D134852

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

@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.




Comment at: clang/lib/Format/Format.cpp:2774
-
-const char CppIncludeRegexPattern[] =
-R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";

kwk wrote:
> MyDeveloperDay wrote:
> > Did I miss where this comes from now?
> @MyDeveloperDay `clang/lib/Tooling/Inclusions/HeaderIncludes.cpp` still has 
> this:
> 
> ```lang=c++
> const char IncludeRegexPattern[] =
> R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
> ```
> 
> And in `clang/lib/Tooling/Inclusions/HeaderIncludes.cpp` @owenpan uses it to 
> initialize the final regex 
> 
> ```lang=c++
> 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](https://reviews.llvm.org/D134733) 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](https://reviews.llvm.org/D134733#3821957). So I favor my solution.
> 
> 
> 
> 
> The fact that we have two regex that are identical is an issue on its own 
> that I tried to address with [my patch](https://reviews.llvm.org/D134733) 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](https://reviews.llvm.org/D134733#3821957). 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134852/new/

https://reviews.llvm.org/D134852

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Konrad Wilhelm Kleine via Phabricator via cfe-commits]

kwk added a comment.

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=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 `<>"`.




Comment at: clang/lib/Format/Format.cpp:2774
-
-const char CppIncludeRegexPattern[] =
-R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";

MyDeveloperDay wrote:
> Did I miss where this comes from now?
@MyDeveloperDay `clang/lib/Tooling/Inclusions/HeaderIncludes.cpp` still has 
this:

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

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

```lang=c++
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](https://reviews.llvm.org/D134733) 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](https://reviews.llvm.org/D134733#3821957). So I favor my solution.






Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134852/new/

https://reviews.llvm.org/D134852

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/Format.cpp:2774
-
-const char CppIncludeRegexPattern[] =
-R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";

Did I miss where this comes from now?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134852/new/

https://reviews.llvm.org/D134852

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Owen Pan via Phabricator via cfe-commits]

owenpan created this revision.
owenpan added reviewers: MyDeveloperDay, kwk, HazardyKnusperkeks, curdeius.
owenpan added a project: clang-format.
Herald added a project: All.
owenpan requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134852

Files:
  clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
  clang/lib/Format/Format.cpp
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp

Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -266,6 +266,8 @@
   return false;
 }
 
+const llvm::Regex HeaderIncludes::IncludeRegex(IncludeRegexPattern);
+
 HeaderIncludes::HeaderIncludes(StringRef FileName, StringRef Code,
const IncludeStyle )
 : FileName(FileName), Code(Code), FirstIncludeOffset(-1),
@@ -274,8 +276,7 @@
   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};
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -2769,13 +2769,6 @@
   }
 }
 
-namespace {
-
-const char CppIncludeRegexPattern[] =
-R"(^[\t\ ]*#[\t\ ]*(import|include)[^"<]*(["<][^">]*[">]))";
-
-} // anonymous namespace
-
 tooling::Replacements sortCppIncludes(const FormatStyle , StringRef Code,
   ArrayRef Ranges,
   StringRef FileName,
@@ -2785,7 +2778,6 @@
   .StartsWith("\xEF\xBB\xBF", 3) // UTF-8 BOM
   .Default(0);
   unsigned SearchFrom = 0;
-  llvm::Regex IncludeRegex(CppIncludeRegexPattern);
   SmallVector Matches;
   SmallVector IncludesInBlock;
 
@@ -2842,7 +2834,7 @@
 
 bool MergeWithNextLine = Trimmed.endswith("\\");
 if (!FormattingOff && !MergeWithNextLine) {
-  if (IncludeRegex.match(Line, )) {
+  if (tooling::HeaderIncludes::IncludeRegex.match(Line, )) {
 StringRef IncludeName = Matches[2];
 if (Line.contains("/*") && !Line.contains("*/")) {
   // #include with a start of a block comment, but without the end.
@@ -3120,8 +3112,8 @@
 
 inline bool isHeaderInsertion(const tooling::Replacement ) {
   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 ) {
@@ -3173,11 +3165,11 @@
 }
   }
 
-  llvm::Regex IncludeRegex = llvm::Regex(CppIncludeRegexPattern);
   llvm::SmallVector Matches;
   for (const auto  : HeaderInsertions) {
 auto IncludeDirective = R.getReplacementText();
-bool Matched = IncludeRegex.match(IncludeDirective, );
+bool Matched =
+tooling::HeaderIncludes::IncludeRegex.match(IncludeDirective, );
 assert(Matched && "Header insertion replacement must have replacement text "
   "'#include ...'");
 (void)Matched;
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -79,6 +79,9 @@
   /// 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) {}
@@ -124,9 +127,6 @@
 
   // All possible priorities.
   std::set Priorities;
-
-  // Matches a whole #include directive.
-  llvm::Regex IncludeRegex;
 };
 
 } // namespace tooling
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits