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.

Sign up for GitHub

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

Jump to bottom

*: remove trailing underscores from header guards #6407

Merged
merged 3 commits into from
Jan 20, 2017
Merged

*: remove trailing underscores from header guards #6407

merged 3 commits into from
Jan 20, 2017

Conversation

OlegHahm
Copy link
Member

Relaces #4626 and fixes #2623.

@OlegHahm OlegHahm added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Newbie-Task-Candidate CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 18, 2017
@OlegHahm OlegHahm mentioned this pull request Jan 18, 2017
Closed
A-Paul
A-Paul reviewed Jan 18, 2017
View reviewed changes
Copy link
Member

@A-Paul A-Paul left a comment

Choose a reason for hiding this comment

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

Every #ifdef FOOBAR_H_ has corresponding #endif /* FOOBAR_H_ */. Although the strings in the endif comments are not functional I think its recommended to correct them too.

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jan 18, 2017
@OlegHahm
Copy link
Member Author

Comment addressed

A-Paul
A-Paul reviewed Jan 18, 2017
View reviewed changes
Copy link
Member

@A-Paul A-Paul left a comment

Choose a reason for hiding this comment

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

It's always a delight that github doesn't allow me to mark line that are not changed. :D
Following is a list of filename:linenumber where the comment in the endif is missing or not matching.
boards/chronos/drivers/include/display.h:466
boards/chronos/include/buttons.h:27
boards/msb-430-common/include/board_common.h:64
boards/pba-d-01-kw2x/include/board.h:135
boards/pba-d-01-kw2x/include/periph_conf.h:296
boards/qemu-i386/include/board.h:40
boards/qemu-i386/include/cpu_conf.h:39
boards/telosb/include/board-conf.h:22
boards/z1/include/board-conf.h:37
cpu/arm7_common/include/VIC.h:92
cpu/arm7_common/include/iap.h:67
cpu/k60/include/system_MK60D10.h:80
cpu/stm32l1/include/cpu_conf.h:45
cpu/x86/include/cpu.h:134
drivers/include/kw2xrf.h:162
drivers/include/sht11.h:108
examples/riot_and_cpp/c_functions.h:50
examples/riot_and_cpp/cpp_class.hpp:63
sys/include/crypto/modes/ctr.h:73
sys/include/crypto/modes/ecb.h:62
sys/include/xtimer/implementation.h:305
sys/posix/pthread/include/pthread_spin.h:94

@OlegHahm
Copy link
Member Author

It's always a delight that github doesn't allow me to mark line that are not changed. :D

Isn't that a good indicator that these issues should be addressed in a separate PR?

@OlegHahm
Copy link
Member Author

But since you were so kind to already provide a list, I've extended the purpose of this PR. :)

@A-Paul
Copy link
Member

A-Paul commented Jan 18, 2017 •
edited
Loading

Then I don't understand why you didn't object my first comment, that was also related to areas you hadn't changed.
Be consistent :D

A-Paul
A-Paul approved these changes Jan 18, 2017
View reviewed changes
Copy link
Member

@A-Paul A-Paul left a comment

Choose a reason for hiding this comment

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

Looks valid to me. ACK when squashed and CI is green.

@miri64
Copy link
Member

miri64 commented Jan 18, 2017

May I ask why we remove the trailing _, too? #2623 was only about leading IIRC.

@OlegHahm
Copy link
Member Author

May I ask why you raise this question after more than one year? ;-)

But anyway, I read #2623 differently and I don't see a good reason for the trailing underscore.

@miri64
Copy link
Member

miri64 commented Jan 18, 2017

May I ask why you raise this question after more than one year? ;-)

Wasn't aware #2623 also included trailing _ ;-)

But anyway, I read #2623 differently and I don't see a good reason for the trailing underscore.

Fine by me, just need to adapt my templates.

@A-Paul
Copy link
Member

A-Paul commented Jan 18, 2017

It's all about balance. ;-)

@miri64
Copy link
Member

miri64 commented Jan 18, 2017

It's all about balance. ;-)

Come to the dark side. We have cookies.

@OlegHahm
Copy link
Member Author

Interesting problem - I'm investigating.

@OlegHahm
Copy link
Member Author

Figured the problem - LWIP has ambiguous header guards.

@miri64
Copy link
Member

miri64 commented Jan 18, 2017

Oops :-/

@OlegHahm OlegHahm force-pushed the header_guards branch 2 times, most recently from f870a06 to 408db23 Compare January 18, 2017 22:46
@OlegHahm OlegHahm removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jan 18, 2017
@OlegHahm
Copy link
Member Author

I must admit, I'm a little bit lost regarding the CI fails.

@jnohlgard
Copy link
Member

@OlegHahm The patches to pkg don't apply, you need to revert the - lines so that they can be applied.
Also you might need to check that the patches are still working as intended, there's some preprocessor ifdefs there which might have been affected.

@cgundogan
Copy link
Member

To complement @gebart's comment. This is what I can read from the Jenkins output:

git -C /opt/jenkins/workspace/RIOT_PR-6407-AJNL6AFS5Q2QDBPR2KWOCIYZ2LMVFJSEIJILOO2BGRBMGFT5EUDA@2/tests/nhdp/bin/pkg/airfy-beacon/oonf_api am --ignore-whitespace "/opt/jenkins/workspace/RIOT_PR-6407-AJNL6AFS5Q2QDBPR2KWOCIYZ2LMVFJSEIJILOO2BGRBMGFT5EUDA@2/pkg/oonf_api"/patches/*.patch
Applying: add RIOT support
Applying: port tests to riot
Applying: port example to riot
Applying: fix conflicting types
Applying: only define container_of when necessary
Applying: if_index is not used
Applying: Use RIOT's container_of implementation
Applying: Dissolve enum into single defines
Applying: Add missing include
Applying: Change index of array from 0 to 1
Applying: remove error: array subscript has type 'char' [-Werror=char-subscripts]
Applying: isolate oonf list implementation of riot's implementation
error: patch failed: src-api/common/list.h:466
error: src-api/common/list.h: patch does not apply
error: patch failed: src-api/subsystems/oonf_packet_socket.h:131
error: src-api/subsystems/oonf_packet_socket.h: patch does not apply
Patch failed at 0012 isolate oonf list implementation of riot's implementation
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
/opt/jenkins/workspace/RIOT_PR-6407-AJNL6AFS5Q2QDBPR2KWOCIYZ2LMVFJSEIJILOO2BGRBMGFT5EUDA@2/pkg/pkg.mk:17: recipe for target '/opt/jenkins/workspace/RIOT_PR-6407-AJNL6AFS5Q2QDBPR2KWOCIYZ2LMVFJSEIJILOO2BGRBMGFT5EUDA@2/tests/nhdp/bin/pkg/airfy-beacon/oonf_api/.git-patched' failed
make[1]: *** [/opt/jenkins/workspace/RIOT_PR-6407-AJNL6AFS5Q2QDBPR2KWOCIYZ2LMVFJSEIJILOO2BGRBMGFT5EUDA@2/tests/nhdp/bin/pkg/airfy-beacon/oonf_api/.git-patched] Error 128
/opt/jenkins/workspace/RIOT_PR-6407-AJNL6AFS5Q2QDBPR2KWOCIYZ2LMVFJSEIJILOO2BGRBMGFT5EUDA@2/tests/nhdp/../../Makefile.include:321: recipe for target '/opt/jenkins/workspace/RIOT_PR-6407-AJNL6AFS5Q2QDBPR2KWOCIYZ2LMVFJSEIJILOO2BGRBMGFT5EUDA@2/jenkins_bin/oonf_api.a' failed
make: *** [/opt/jenkins/workspace/RIOT_PR-6407-AJNL6AFS5Q2QDBPR2KWOCIYZ2LMVFJSEIJILOO2BGRBMGFT5EUDA@2/jenkins_bin/oonf_api.a] Error 2
make: Leaving directory '/opt/jenkins/workspace/RIOT_PR-6407-AJNL6AFS5Q2QDBPR2KWOCIYZ2LMVFJSEIJILOO2BGRBMGFT5EUDA@2/tests/nhdp'

@OlegHahm
Copy link
Member Author

Yes, thanks. I somehow overlooked that I've actually touched the patch files.

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jan 19, 2017
@cgundogan
Copy link
Member

seems to be in need of a rebase

@OlegHahm
Copy link
Member Author

Rebased.

@cgundogan
Copy link
Member

FYI: Jenkins is happy with the changes, it just complains about the squashing.

@OlegHahm OlegHahm removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jan 19, 2017
@aabadie aabadie mentioned this pull request Jan 19, 2017
Merged
@OlegHahm
Copy link
Member Author

For some strange reason the header for the NRF softdevice package was complete shit. Fixed it.

@miri64
Copy link
Member

miri64 commented Jan 19, 2017

For some strange reason the header for the NRF softdevice package was complete shit. Fixed it.

Maybe because the review for it happened crammed in the FU lecture hall during RIOT summit ^^.

@miri64 miri64 merged commit a5bdf0a into RIOT-OS:master Jan 20, 2017
@miri64
Copy link
Member

miri64 commented Jan 20, 2017

FYI: there are still headers with trailing _ guards that were merged from PRs that were merged before this PR was last forked/rebased.

@miri64
Copy link
Member

miri64 commented Jan 20, 2017

(and of course all open PRs have the potential to re-introduce #2623 again)

@OlegHahm OlegHahm deleted the header_guards branch January 20, 2017 08:13
@OlegHahm OlegHahm mentioned this pull request Jan 20, 2017
Closed
@aabadie aabadie mentioned this pull request Jan 20, 2017
Merged
@PeterKietzmann PeterKietzmann mentioned this pull request Jan 20, 2017
Merged
@kb2ma kb2ma mentioned this pull request Jan 21, 2017
Merged
@miri64 miri64 mentioned this pull request Jan 23, 2017
Closed
@miri64 miri64 added the Community: good first issue This issue is good for newcomers to RIOT to be addressed / implemented label Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@A-Paul A-Paul A-Paul approved these changes

Assignees

@A-Paul A-Paul

Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: good first issue This issue is good for newcomers to RIOT to be addressed / implemented Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Repair header file include guards
5 participants
@OlegHahm @A-Paul @miri64 @jnohlgard @cgundogan