Skip to content

Fix check_style.sh regex patterns to properly detect style issues#16423

Merged
r0qs merged 3 commits intodevelopfrom
fix-style-check-exclusion-pattern
Feb 2, 2026
Merged

Fix check_style.sh regex patterns to properly detect style issues#16423
r0qs merged 3 commits intodevelopfrom
fix-style-check-exclusion-pattern

Conversation

@r0qs
Copy link
Copy Markdown
Member

@r0qs r0qs commented Jan 26, 2026

While working on #16266, I noticed that some style issues were not being caught by our script. This PR fixes issues in check_style.sh that were causing:

  1. grep warnings due to invalid or unnecessary regex syntax when using the Extended Regular Expressions (-E) option
  2. Real style violations being incorrectly excluded from detection

The old patterns ^* [*] and ^*//.* were flagged as invalid regex by grep (i.e., * at start of expression) and inadvertently matched any line containing * (space followed by asterisk). This caused real pointer alignment issues like CFGNode *_node to be excluded from detection.

The new pattern :[0-9]+:\s*\*\s only excludes comment continuation lines (where the source starts * followed by whitespace), allowing real style violations to be properly detected.

I also changed \/(\/|\*) to /[/*]. Forward slashes are not special characters in regular expressions and do not need escaping. The unnecessary escaping was causing grep: warning: stray \ before / to appear in the script output.

@r0qs
Copy link
Copy Markdown
Member Author

r0qs commented Jan 26, 2026

I think this PR highlights the fragility of our current regex-based approach to style checking. The patterns were silently broken, causing real violations to go undetected while producing confusing warnings. This approach is inherently error-prone and difficult to maintain.

This is another reason to reconsider adopting clang-format as proposed in #15226. While there are differing opinions among team members about specific formatting choices, having an automated and reliable solution is preferable to maintaining brittle regex patterns that can fail silently.

@r0qs r0qs self-assigned this Jan 26, 2026
@cameel
Copy link
Copy Markdown
Collaborator

cameel commented Jan 28, 2026

This is another reason to reconsider adopting clang-format as proposed

There is nothing to reconsider. We already agreed that we want to switch to an automatic formatter when that's good enough to give us something reasonably close to our (Rust-like) style. Last time we checked, clang-format was too Google-style-centric and the results were not satisfactory. But there were patches going in and the situation was supposedly improving so it could be better now. IIRC someone from the team volunteered to try to check it again.

Looks like no progress since then though, which I guess is understandable given that this is far from being a priority and for the last half year+ we've been severely understaffed. But still, there's nothing to decide here. Someone just has to do it.

This approach is inherently error-prone and difficult to maintain.

It's fine for what it is IMO. It's not meant to be a full style-checker. It just catches low-hanging fruit. TBH I would have expected there to be far more breakage here given that we pretty never touch it. It may be difficult to maintain but we cannot honestly say we're really maintaining it :P We're just letting it rot because everyone is looking forward to a formatter.

@clonker
Copy link
Copy Markdown
Member

clonker commented Jan 28, 2026

There is nothing to reconsider.

We might consider just switching styles to something supported.

IIRC someone from the team volunteered to try to check it again.

That was me, a while ago, couldn't get it to match our style though.

Someone just has to do it.

Someone can look into it again but really I think we should be focusing more on code and less on style.

We're just letting it rot because everyone is looking forward to a formatter.

Let's use one, then? :)

@r0qs r0qs merged commit 8b76e39 into develop Feb 2, 2026
84 checks passed
@r0qs r0qs deleted the fix-style-check-exclusion-pattern branch February 2, 2026 10:53
@cameel
Copy link
Copy Markdown
Collaborator

cameel commented Feb 3, 2026

I think this is putting the priorities wrong. Having readable code is the goal, automating it should come second. Definitely nice to have, but not at the expense of the former.

I mean, I know it can be annoying and gets more spotlight when you run into bugs like this one, but let's put things into perspective. Realistically, I just don't believe this is anywhere near the top things that are holding us back. With how easy it is to produce code these days (IDEs, refactoring tools, AI) I find that optimizing for reading is an orders of magnitude bigger concern so anything that comes with even a minimal degradation there is just not worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants