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

updated makefiles & a handler for the heap state shell command #64

Closed
wants to merge 0 commits into from

Conversation

zkasmi
Copy link
Contributor

@zkasmi zkasmi commented Jul 1, 2013

Updated makefiles allowing a shell working with spaces in paths. And a handler for the heap state shell command.

extern void heap_stats(void);

void _heap_handler(char* unnused){
heap_stats();
Copy link
Contributor

Choose a reason for hiding this comment

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

you are just adding one line of production code. please remove all "windows compat lines" and create one (1) commit and a new pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ø you are just adding one line of production code

one “Line Of Production code” follows the same scheme as in “sc_ps.c”, “sc_id.c.”, etc.

In sys/shell/commands/sc_heap.c:

+extern void heap_stats(void);

+void _heap_handler(char* unnused){

  • heap_stats();
    

you are just adding one line of production code. please remove all "windows compat lines" and create one (1) commit and a new pull request.


Reply to this email directly or view it on GitHubhttps://github.com//pull/64/files#r5195888.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one “Line Of Production code” follows the same scheme as in “sc_ps.c”, “sc_id.c.”, etc.

In sys/shell/commands/sc_heap.c:

+extern void heap_stats(void);

+void _heap_handler(char* unnused){

  • heap_stats();
    

you are just adding one line of production code. please remove all "windows compat lines" and create one (1) commit and a new pull request.


Reply to this email directly or view it on GitHubhttps://github.com//pull/64/files#r5195888.

Copy link
Contributor

Choose a reason for hiding this comment

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

On 07/16/2013 11:35 AM, Zakaria Kasmi wrote:

In sys/shell/commands/sc_heap.c:

+extern void heap_stats(void);
+
+void _heap_handler(char* unnused){

  • heap_stats();

one “Line Of Production code” follows the same scheme as in “sc_ps.c”, “sc_id.c.”, etc.
This might be correct, but the commits introducing “sc_ps.c”,
“sc_id.c.”, etc. didn't introduce "Makefile fixes".

If we pull your one-liner together with the (unrelated) makefile updates, there'll
be a "merge commit". Now imagine your one line has a problem somewhere
else, and we revert the merge commit. That would also unmerge your
Makefile "fixes". (Makefile updates adding your .c file is totally OK)

Christian proposed to seperate the makefile fixes from your added
functionality.

@zkasmi
Copy link
Contributor Author

zkasmi commented Jul 16, 2013

what is meant by ""windows compat lines"? i find no lines !!!

@kaspar030
Copy link
Contributor

On 07/16/2013 12:14 PM, Zakaria Kasmi wrote:

what is meant by ""windows compat lines"? i find no lines !!!
No sane OS has spaces in it's path to make, so we were guessing...

@zkasmi
Copy link
Contributor Author

zkasmi commented Jul 16, 2013

It is inconvenient for the Windows-user each time to change $(MAKE) to "$(MAKE)". Otherwise, the updated makfiles include no windows-specific commands.

On 07/16/2013 12:14 PM, Zakaria Kasmi wrote:

what is meant by ""windows compat lines"? i find no lines !!!
No sane OS has spaces in it's path to make, so we were guessing...


Reply to this email directly or view it on GitHubhttps://github.com//pull/64#issuecomment-21034701.

@kaspar030
Copy link
Contributor

On 07/16/2013 01:23 PM, Zakaria Kasmi wrote:

It is inconvenient for the Windows-user each time to change $(MAKE) to
"$(MAKE)".
Otherwise, the updated makfiles include no windows-specific commands.
The "$(MAKE)" changes might be useful, they just don't belong in the
same pull request as your sc_heap stuff. Thanks for confirming our
windows guess.

@OlegHahm
Copy link
Member

OlegHahm commented Aug 6, 2013

Updated by #93 and #94

@OlegHahm OlegHahm closed this Aug 6, 2013
nmeum added a commit to beduino-project/RIOT that referenced this pull request Apr 2, 2017
By including an ffi.rs file in the crate directory.

Fixes RIOT-OS#64
nmeum added a commit to beduino-project/RIOT that referenced this pull request Apr 2, 2017
Two small bug fixes

Closes RIOT-OS#52 and RIOT-OS#64

See merge request !31
eduazocar pushed a commit to eduazocar/RIOT that referenced this pull request Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants