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

Wed Oct 5 08:14:57 PDT 2022

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&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.
> > 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. :)
> > 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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134852