[PATCH] D134733: [clang-format][chore] transparent #include name regex

[Konrad Wilhelm Kleine via Phabricator via cfe-commits]

kwk added inline comments.


================
Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:407
+    const llvm::SmallVectorImpl<llvm::StringRef> &Matches) {
+  if (Matches.size() >= 3) {
+    return Matches[2];
----------------
MyDeveloperDay wrote:
> kwk wrote:
> > MyDeveloperDay wrote:
> > > ‘>= 2’
> > @MyDeveloperDay correct me if I'm wrong but if an array has size `3` it has indexes `0`, `1` and `2`. And an array of size `2` only has `0` and `1`.  So the case `=2`, which is implied by your suggested`>=2`, is actually an out of bounds access when going `Matches[2]`.  Because that is effectively accessing the third element. The only valid change would be to check for `Matches.size() > 2` IMHO and that is the same as `Matches.size() >= 3`.
> > 
> > I must admit that I had to look at the regex a few times only to realize that it has two non-optional matching groups `(...)`. The third matching group at index `0` is the whole line. So in theory `>=3` isn't possible with the current regex. I wanted to give my best to have this logic "survive" a change to the regex in which for example something is added after the matching group of the include name; let's say a comment or something.
> > 
> > I hope I haven't made myself a complete fool.  
> No foolery other than my own. I meant to say '> 2',  actually the first time I wrote the comment it disappear because it didn't escape the '>'
> 
> So one though I had for the config was something like this (ignore regex itself I just put anything in it to show the principle)
> 
> ```
> ---
> Language Cpp
> IncludeRegex:  ^[\t\ ]*[@#][\t\ ]*(import|include)([^"/]*("[^"]+")|[^</]*(<[^>]+>)|[\t/\ ]*([^;]+;)
> IncludeRegexGroup: 1
> ---
> Language: ObjectiveC
> IncludeRegex:  ^[\t\ ]*[@#][\t\ ]*(import|include)([^"/]*("[^"]+")|[^</]*(<[^>]+>)|[\t/\ ]*([^;]+;)
> IncludeRegexGroup: 2
> ---
> Language: CSharp
> IncludeRegex:  using  ([A-z])*
> IncludeRegexGroup: 0
> ---
> Language: Carbon
> IncludeRegex:  package ([A-z])* api
> IncludeRegexGroup: 0
> ---
> 
> ```
> 
> Wouldn't this allow different languages to have their own Include/Import regex? Just thinking out loud
> 
> It could be more powerful than having 1 regex to rule them all.
> 
> 
> 
> 
@MyDeveloperDay I like the idea of this approach! But I also see that a single language alone, namely C++, is quite a tough nut to crack with the advent of Modules (https://en.cppreference.com/w/cpp/language/modules). I'm afraid the bit of `IncludeRegexGroup` would need to be expanded to be either a

* **fixed number** (wherever possible)
* a **method** as in: last-non-empty-matching-group

Only then I think that your solution would work. Can one have a setting in clang format that allows for mixed input as in number or string? A bit of a hacky solution but probably cutting 100% of all regexes would be to have an enum with these allowed strings: `last-non-empty-matching-group,first-non-empty-matching-group,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134733