[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 <https://reviews.llvm.org/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