Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dist: tools: add headerguard check script #7095

Merged
merged 5 commits into from
May 24, 2017

Conversation

kaspar030
Copy link
Contributor

This PR adds a python script checking for header guards.

Features:

  • it outputs diffs that can be directly applied.
  • it fixes / adds the commend at the closing #endif
  • it adds the file path to the guard, up to include/, or none if there's no include in the path. (e.g., 'sys/include/a/b/c.h' -> 'A_B_C_H'), 'sys/posix/blah/foo.h' -> 'FOO_H'

One commit adds some missing header guards (changes made manually),
another commit applies the headercheck output to the codebase.

@kaspar030 kaspar030 added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: tools Area: Supplementary tools labels May 23, 2017
@kaspar030 kaspar030 force-pushed the add_headerguard_check_script branch from 54768cb to 7aace34 Compare May 23, 2017 16:20
@kaspar030
Copy link
Contributor Author

  • force-pushed last-minute-fix

@miri64
Copy link
Member

miri64 commented May 23, 2017

it adds the file path to the guard, up to include/, or none if there's no include in the path. (e.g., 'sys/include/a/b/c.h' -> 'A_B_C_H'), 'sys/posix/blah/foo.h' -> 'FOO_H'

Why? I think that puts many constraints on the filename. E.g. if I have a module blafoo, I want to call the header sys/include/blafoo.h and the interna header sys/blafoo/_blafoo.h. If your script works as I expect it to be, this wouldn't be possible, since the leading underscore is removed, right?

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 24, 2017
@kaspar030
Copy link
Contributor Author

sys/blafoo/_blafoo.h. If your script works as I expect it to be, this wouldn't be possible, since the leading underscore is removed, right?

No, the script doesn't remove underscores that are in the filename. _blafoo.h would lead to _BLAFOO_H as header guard.

@miri64
Copy link
Member

miri64 commented May 24, 2017

No, the script doesn't remove underscores that are in the filename. _blafoo.h would lead to _BLAFOO_H as header guard.

I thought we agreed that this is not allowed Oo. @OlegHahm and @gebart IIRC you were the main people behind this coding convention.

@jnohlgard
Copy link
Member

@miri64 do we have any header files with leading underscores in the file name, or is this a non-issue?

@miri64
Copy link
Member

miri64 commented May 24, 2017

@miri64 do we have any header files with leading underscores in the file name, or is this a non-issue?

A quick find told me no, but this script doesn't check, if a guard has leading underscores (which was the intent behind #6438 AFAIK). This would re-introduce such guards (which we removed in a quite lengthy process) as soon one decides to add such header files.

@kaspar030
Copy link
Contributor Author

This would re-introduce such guards

As of now, it wouldn't, because we don't have header filenames with a leading _.
How would you name the guard of such a file?
This PR is also about finding simple rules that can be enforced.

I was actually surprised how many guards were wrong (copy&pasta, file renames), or which had wrong comments at the closing #endif...

@miri64
Copy link
Member

miri64 commented May 24, 2017

How would you name the guard of such a file?
This PR is also about finding simple rules that can be enforced.

This won't solve potential collisions, but how about _blafoo.h => PRIV_BLAFOO_H?

@kaspar030
Copy link
Contributor Author

This won't solve potential collisions, but how about _blafoo.h => PRIV_BLAFOO_H?

I'll add that.
Actually there's only be a collision if two headers with the same name are in the include paths, meaning there's already a problem. AFAIK the local directory would have precedence in that case.

@kaspar030
Copy link
Contributor Author

  • addressed leading "_" comment
  • added logic to return error if there was a broken/missing header guard

@kaspar030
Copy link
Contributor Author

  • fixed error exit code logic

@kaspar030 kaspar030 requested a review from miri64 May 24, 2017 13:30
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this enforces a new naming rule for header rules, please update also the coding conventions accordingly.

Also I'm unsure if replacing _ and / to the same character string might lead to future problems, but since this is a future problem leave this task to future us ;-).

https://www.youtube.com/watch?v=W9kH7bn0CgM

@@ -15,8 +15,8 @@
*
* @author Martine Lenders <mlenders@inf.fu-berlin.de>
*/
#ifndef RIOT_INTTYPES_H
#define RIOT_INTTYPES_H
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assorted musings / talking while thinking: mhhhh a RIOT prefix might make sense (but let's keep this patch as simple as possible and go with the more established solution)

_in = "/-."
_out = "___"

transtab = str.maketrans(_in, _out)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I learned a new python thing \o/ (I usually use re.sub() for that).

@miri64
Copy link
Member

miri64 commented May 24, 2017

(please squash)

@kaspar030 kaspar030 force-pushed the add_headerguard_check_script branch from 27a7cc8 to 60fb6d2 Compare May 24, 2017 15:54
@kaspar030
Copy link
Contributor Author

Since this enforces a new naming rule for header rules, please update also the coding conventions accordingly.

Done.

@miri64 miri64 merged commit d469364 into RIOT-OS:master May 24, 2017
@kaspar030
Copy link
Contributor Author

Thanks for reviewing!

@kaspar030 kaspar030 deleted the add_headerguard_check_script branch May 24, 2017 16:08
@aabadie aabadie modified the milestone: Release 2017.07 Jun 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants