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