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

board cpu and core: header file include guards repaired #4626

Closed
wants to merge 2 commits into from

Conversation

rajma996
Copy link

@rajma996 rajma996 commented Jan 9, 2016

pull request created as per the issue #2623
changes made in board directory and some files in cpu directory

#ifndef CHRONOS_BOARD_H_
#define CHRONOS_BOARD_H_
#ifndef CHRONOS_BOARD_H
#define CHRONOS_BOARD__
Copy link
Member

Choose a reason for hiding this comment

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

something went wrong here.
s/CHRONOS_BOARD__/CHRONOS_BOARD_H/

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing, it's resolved now.

@jnohlgard
Copy link
Member

I am not sure of the implications of the changes to the CMSIS headers (cpu/cortexm_common/include/core_*.h), and __CMSIS_GENERIC is not a traditional include guard and should be left as-is.
Changing the CMSIS headers increases maintenance when upgrading CMSIS releases as well so I propose to just leave them as they are right now.

The SAMxxx headers are missing a trailing _H to match the pattern of all other include guards in the tree.

@PeterKietzmann PeterKietzmann added the Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation label Jan 11, 2016
@rajma996 rajma996 changed the title board and some files in cpu : header file include guards repaired board ,cpu and core: header file include guards repaired Jan 12, 2016
@rajma996 rajma996 changed the title board ,cpu and core: header file include guards repaired board cpu and core: header file include guards repaired Jan 12, 2016
@Kijewski
Copy link
Contributor

rm core/include/cscope.out

@BytesGalore
Copy link
Member

I'm with @gebart, since the CMSIS headers define an open standard they are subject of changes and probably will change in the future.
So we should keep them untouched for seamless upgrade and maintenance.

#ifndef __PIPE__H
#define __PIPE__H
#ifndef PIPE__H
#define PIPE__H

Copy link
Member

Choose a reason for hiding this comment

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

could you change also the #endif in L122 to #endif /* PIPE__H */?

@BytesGalore BytesGalore 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, 2016
@BytesGalore
Copy link
Member

@rajma996 beside the discussion on the CMSIS headers and my request for adding a comment your changes looks good 👍
Since you started to adjust the headers in sys/ I propose to make a separate PR for them and go for merging this PR soon.
So I would say please address our requests and squash your commits.

@rajma996
Copy link
Author

@BytesGalore Thanks for the feedback. CMSIS headers are restored. As I will be making a separate PR for /sys headers, changes in the sys/include/pipe.h will be done there :)

@BytesGalore
Copy link
Member

@rajma996 I would say, please squash the 15 commits of this RP into a single one :)
And then lets see what travis says.

@BytesGalore
Copy link
Member

@rajma996 please rebase and squash the commits :)

@rajma996
Copy link
Author

@BytesGalore sorry for being late, I tried some wierd method to sync with the main repo... I've reverted back to the last header updating commit. should I squash from the commit I first started editing headers.

@BytesGalore
Copy link
Member

@rajma996 don't know if you strictly separated your changes in the individual commits, but it would be nice if you could squash to 3 commits like:

  • boards: repaired header guards
  • cpu: repaired header guards
  • core: repaired header guards

If this is not possible, no problem :) then just squash all your changes to one commit and rename it accordingly, e.g. boards/cpu/core: repaired header guards.
You also need to rebase this PR to the current master, if you have troubles with it just ask :)

@rajma996
Copy link
Author

@BytesGalore thanks for the reply. I reverted all the commits I used for syncing with the main repo previously and squashed everything from the beginning of this forked repo. Let me try rebasing with the current master. I will keep you updated. Again thanks :D

@rajma996
Copy link
Author

@BytesGalore though changes in the commit looks fine, but Travis checks have failed....:(

@@ -6842,7 +6842,11 @@ typedef struct
}
#endif /* __cplusplus */

<<<<<<< HEAD:cpu/stm32f1/include/stm32f103xe.h
Copy link
Member

Choose a reason for hiding this comment

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

I guess this one slipped through :)
(That's what travis complains)

@BytesGalore
Copy link
Member

oh and please reword the commit message to something like:
board cpu core: header file include guards repaired

@BytesGalore BytesGalore removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 17, 2016
@BytesGalore BytesGalore added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 17, 2016
@BytesGalore
Copy link
Member

cool 👍 then lets kick travis

#ifndef __ERRNO_H_
#define __ERRNO_H_ 1
#ifndef ERRNO_H
#define ERRNO_H 1

Copy link
Member

Choose a reason for hiding this comment

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

I guess this guy needs to remain with underscores

@rajma996
Copy link
Author

@BytesGalore build wasn't successful this time, but i am not able to understand the error.... looks like it couldn't connect for updates.

@BytesGalore
Copy link
Member

@rajma996 seems unrelated I kicked travis to repeat the failed build

@rajma996
Copy link
Author

@BytesGalore build is successful. :D

@BytesGalore
Copy link
Member

nice one, ACK from my side 👍
Since this PR touches so much files, also in core, I would like to have another ACK before we merge it.
@gebart, @PeterKietzmann would you take a look?

@OlegHahm OlegHahm added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Feb 27, 2016
@rajma996
Copy link
Author

@BytesGalore Based on previous requests on PR 4616 I should've made separate commit for each directory instead of squashing all into one. Let me work on that now...

@miri64
Copy link
Member

miri64 commented Mar 1, 2016

Please rebase.

@miri64 miri64 modified the milestone: Release 2016.07 Mar 29, 2016
@miri64
Copy link
Member

miri64 commented May 17, 2016

Still needs rebase.

@rajma996
Copy link
Author

@BytesGalore I am a bit stuck here, I tried rebasing but wasn't successful :( Can you help...

@BytesGalore
Copy link
Member

@rajma996 ok I see your changes are now made on your master branch, which is not what you want.
It seems you additionally merged commits/changes from RIOT master to your commits.
But don't worry we can manage it :)

The goal is to keep the changes on the 109 headers but throw all other changes to get in sync with the original RIOT master again.
So we first clone your RIOT, checkout the master branch and switch to the RIOT directory to start.
(Just to prevent uncontrolled heat/sweating, backup the folder before starting.)

  1. We remember the filenames of the 109 headers you edited:
    git diff --name-only b986ac33e81f89acf07a6ca814b7cd7cdffb109a >> ../keep_em_all.txt
    This will create a file keep_em_all.txt in the above directory containing the relative paths + filenames of the 109 headers.
  2. we go back 2 commits, since your commit seems to be merged on top of the former one:
    git reset HEAD~2
    If you type git show it should list the changes for your 109 edited header files plus a large bunch of other changed files + untracked ones.
    If you type git log you should see commit b986ac33e81f89acf07a6ca814b7cd7cdffb109a on top, which is the first one in sync with RIOT.
  3. we open a new branch to move our changes and clean-up main:
    git checkout -b repaired_header_guards
  4. now we add only the 109 headers we kept in keep_em_all.txt to commit them:
    for word in $(cat ../keep_em_all.txt); do git add $word; done
    If you type git status you should see (when you scroll some pages up) the 109 header files added for being committed.
  5. Then lets commit them:
    git commit -m "core/board/cpu: repaired header guards"
    If you type git status again, only the other changed/untracked files will be listed.
  6. Now we want to get rid of them, so we checkout their original versions:
    for word in $(git diff --name-only b986ac33e81f89acf07a6ca814b7cd7cdffb109a); do git checkout $word; done
    This time we didn't needed to remember their filenames, so we processed them directly.
    If you type git status you should see only as untracked marked files.
  7. Now we switch to master to clean it up completely:
    git checkout master
    and then stash the untracked marked files:
    git stash
    stashes the files and finally allows us to sync your master with the original RIOT master
    (basically you can delete the stash afterwards since the changes are introduced back after rebase)
  8. Now we rebase your master:
    git pull --rebase https://github.com/RIOT-OS/RIOT.git
  9. After that you can rebase your branch repaired_header_guards:
    git checkout repaired_header_guards
    git rebase master
    fixing appearing conflicts... ... ...
  10. and finally pushing it to your repo:
    git push -u origin repaired_header_guards
    and
    git push -f for your master branch.
    Note: the -f forces to overwrite your remote repo stuff with the local changes, so be sure everything works as expected since there's no way back, once overwritten things are gone.

Since you have then a new branch we need to find out how to "connect" it to this PR, or we just open a new PR and close this one.

Sorry for the wall of text, I hope this helps a bit :)

@BytesGalore BytesGalore removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 31, 2016
@kYc0o kYc0o removed the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Jul 25, 2016
@kYc0o
Copy link
Contributor

kYc0o commented Jul 25, 2016

Can you rebase please? This will be reviewed for the next release.

@kYc0o kYc0o modified the milestones: Release 2016.10, Release 2016.07 Jul 25, 2016
@rajma996
Copy link
Author

@kYc0o sorry for the delay.

@@ -0,0 +1,91 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

This file was removed in master, so please don't re-include it.

@miri64
Copy link
Member

miri64 commented Oct 31, 2016

Postponed due to feature freeze

@miri64 miri64 modified the milestones: Release 2017.01, Release 2016.10 Oct 31, 2016
@A-Paul
Copy link
Member

A-Paul commented Dec 20, 2016

@rajma996, ping :-)

@OlegHahm
Copy link
Member

superseded by #6407.

@OlegHahm OlegHahm closed this Jan 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

10 participants