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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay 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];
----------------
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.






Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134733