[Docs] Remove rule about matching .c and .h files #861
Conversation
oshogbo
force-pushed
the
oshogbo/code_style
branch
from
4e3c624
to
d7e29a1
Compare
oshogbo
changed the title
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @oshogbo)
a discussion (no related file):
Given that most people are in favor of this we probably merge it, but let's hear from @mkow as he was most advocating for this rule.
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @mkow)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
Given that most people are in favor of this we probably merge it, but let's hear from @mkow as he was most advocating for this rule.
Looking at the list of files in this PR, we surely ignored this rule most of the time :)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @boryspoplawski)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Looking at the list of files in this PR, we surely ignored this rule most of the time :)
As I said in #850, this rule was inherited from Google C++ styleguide and I rather like it, because the matching header is more "special" than others on the includes list - it contains e.g. forward declarations for functions defined in that .c file.
If you really don't like it then we can remove it, although I'd rather prefer to fix the places which don't follow this rule ;)
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
As I said in #850, this rule was inherited from Google C++ styleguide and I rather like it, because the matching header is more "special" than others on the includes list - it contains e.g. forward declarations for functions defined in that .c file.
If you really don't like it then we can remove it, although I'd rather prefer to fix the places which don't follow this rule ;)
I vote for removing this rule - it is indeed a bit more special, but I don't this having it at the beginning "adds any value" - IMO it's just useless, so there is no point in bothering (the less useless rules the better).
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)
a discussion (no related file):
If you really don't like it then we can remove it, although I'd rather prefer to fix the places which don't follow this rule ;)
But it seems (based on the changes on this PR) that 95% of the files do NOT use this rule. Which means that developers ignore this rule. So we're making life of developers harder for no particular reason...
boryspoplawski
force-pushed
the
oshogbo/code_style
branch
from
d7e29a1
to
c859ab6
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL)
There was a problem hiding this comment.
Reviewed 1 of 7 files at r1.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)
a discussion (no related file):
it seems (based on the changes on this PR) that 95% of the files do NOT use this rule
No, I think the reason that this PR is small is that most of our .c files don't have a corresponding .h file, but instead have these super-headers likeapi.h or pal.h. So, it rather shows a problem with the mess in our codebase :)
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
it seems (based on the changes on this PR) that 95% of the files do NOT use this rule
No, I think the reason that this PR is small is that most of our .c files don't have a corresponding .h file, but instead have these super-headers like
api.horpal.h. So, it rather shows a problem with the mess in our codebase :)
Most of the .c files in LibOS do have a corresponding header.
boryspoplawski
force-pushed
the
oshogbo/code_style
branch
from
c859ab6
to
c57e619
Compare
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL)
Signed-off-by: Mariusz Zaborski <[email protected]>
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
Description of the changes
@dimakuv and @boryspoplawski in #850 proposed to remove a rule about matching .c and .h files.
I went through all files in
pal/src,pal/regression,libos/src/,libos/test/abi,libos/test/fs,libos/test/regression, and there subdirectories - it seems like this rule isn't applied often.How to test this PR?
Build should be enough.
This change is