dist tools: header guard checker #6438
Conversation
OlegHahm
added
Area: CI
| EXIT_CODE=0 | ||
|
|
||
| # check files | ||
| if FILES=$(git grep -lE 'ifndef\ +(_.*|.*_H_$)' ${DIRS}) |
There was a problem hiding this comment.
I think this is too broad. #ifndef __APPLE__ or #ifndef __WITH_AVRLIBC__ will also be triggered as an error with this regex. How about ^#ifndef\ +(_.*_H$|.*_H_$)?
There was a problem hiding this comment.
Oops, I already had this in my local copy, but forgot to push it.
| # directory for more details. | ||
|
|
||
| # customizable | ||
| DIRS="core drivers pkg sys" |
There was a problem hiding this comment.
why not boards? If we do the proposed vendor/ include structure, we might also be able to add cpu.
There was a problem hiding this comment.
True. I saw some warnings and thought they were vendor specific, but actually they can and should be fixed. Will update.
| EXIT_CODE=0 | ||
|
|
||
| # check files | ||
| if FILES=$(git grep -lE 'ifndef\ +(_.*_H$|.*_H_$)' ${DIRS}) |
There was a problem hiding this comment.
Why not ^# *ifndef? Why not -n instead of -l (I think it's actually beneficial to see line number and the offending #ifndef as well)
There was a problem hiding this comment.
Why not ^# *ifndef?
What's the advantage?
Why not -n instead of -l (I think it's actually beneficial to see line number and the offending #ifndef as well)
My assessment was exactly the opposite. Too much cluttered information for my taste.
There was a problem hiding this comment.
What's the advantage?
Avoiding weird false-positives like e.g.
const struct someweirdtypenameifndef _MY_PRIVATE_CONSTANT_FOR_HEIGHT_HMy assessment was exactly the opposite. Too much cluttered information for my taste.
I'm already annoyed about the doc-checker, that tells me there is a problem in file x, but not what the problem is ;-)
|
Mhhh this script can still be "tricked" if a contributor changes the ifndef, but not the define or the comment at the endif ;-) |
Yes, but if you change the ifndef and not the define the header guards is broken anyway. And regarding the endif - I have no idea how to check this in a sensible way. Suggestions? |
Instinctively I would say to check for |
|
But I still have to verify that it matches the correct ifndef. |
|
Take it from the results of that search? |
|
Is it safe to assume that the first ifndef is always the header guard? |
|
I would expect it to be, but I might be wrong. I don't see any functional requirement for such a thing, so we could just make it a convention. |
I'd say, fairly safe if the following line contains the corresponding define. |
|
Damn, this script seems to get way more complicated than initially intended. |
|
Sorry, I won't have the time to address all the comments today. |
OlegHahm
added
the
CI: needs squashing
|
Needs rebase. |
|
I guess this is something coccinelle can take over, right @kaspar030? |
Nope. Coccinelle doesn't care about file names, define values, etc. |
|
I got bored today... -> #7095 |
|
since #7095 was merged, we can close this one (re-open if disagree). |
miri64
added
Community: good first issue
Addressing #6407 (comment):
fix freshly introduced header guards and providing a script to avoid reintroducing wrong guards (at least in main folders).