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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
