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

native: prettify argument parsing #6337

Merged
merged 1 commit into from
Jan 18, 2017

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Jan 12, 2017

I always found it quite weird how native parses it's command-line arguments. This PR moves that to libc's getopt_long(). According to my research this should also be available on OSX and BSD. I also cleaned up some weird state settings (using strings instead of enums oO, what was that about).

Reminder to self: fix #6121 and #6311 for this if this PR gets merged.

@miri64 miri64 added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Platform: native Platform: This PR/issue effects the native platform labels Jan 12, 2017
@miri64 miri64 added this to the Release 2017.01 milestone Jan 12, 2017
@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 12, 2017
@miri64 miri64 force-pushed the native/enh/pretty-startup branch 2 times, most recently from 151ec76 to 44240e3 Compare January 12, 2017 22:37
@LudwigKnuepfer
Copy link
Member

LudwigKnuepfer commented Jan 14, 2017

using strings instead of enums oO, what was that about

Honestly I can't remember why I chose to do so. Probably I was just too lazy. The reason why I did not change it is that it does not make that much of a difference IMHO. It's not like this is on a hot path or something ;) Another reason might be that historically this information was also used in a different module and this made the internal API simpler. Look at the git history if you are really interested and not just trying to troll me :-p
In any case this is an improvement, so thank you :)

@LudwigKnuepfer
Copy link
Member

You have my general blessing for making the long due switch to getopt per se ;)
Code looks sane as well.

However I do not have time to carefully test, or capacity to think through it though.
Please pick someone else for reviewing.

@miri64 miri64 assigned OlegHahm and unassigned LudwigKnuepfer Jan 14, 2017
@miri64
Copy link
Member Author

miri64 commented Jan 14, 2017

Look at the git history if you are really interested and not just trying to troll me :-p

Not trying to troll you, just honest bewilderment. Seems like it was laziness after all (or because of stuff you never commited), since it is like this basically since its inception in 864267f

@LudwigKnuepfer
Copy link
Member

Actually this is what I had in mind when I wrote:

Another reason might be that historically this information was also used in a different module and this made the internal API simpler.

Modules are cpu and board in this case.

@miri64 miri64 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 Jan 18, 2017
@OlegHahm
Copy link
Member

Did someone check on BSD or OSX? @kYc0o, @emmanuelsearch, @smlng?


switch (stdiotype) {
case _STDIOTYPE_STDIO:
return -1;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you change this to a nice ERRNO?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's native internal and I just check if an error accured. I don't see the point to use an errno here.

Copy link
Member

Choose a reason for hiding this comment

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

I thought it would ease refactoring to have macro constant instead of a literal, but no strong opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just this internal function and the check is either <0 or =0, so even if there is refactoring needed in the future, it shouldn't be that extensive. ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Remember: we are talking about the error-handling of some file-internal function that executes commands by a value that was set due to user input. ;-)

Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

just tested (a little) on macOS: works

@miri64
Copy link
Member Author

miri64 commented Jan 18, 2017

just tested (a little) on macOS: works

what does "a little" mean, did you check long options and short e.g.?

@OlegHahm
Copy link
Member

OlegHahm commented Jan 18, 2017

just tested (a little) on macOS: works

what does "a little" mean, did you check long options and short e.g.?

From Cambridge Dictionary:

(A) little and (a) few are quantifiers meaning ‘some’. Little and few have negative meanings. We use them to mean ‘not as much as may be expected or wished for’.

@miri64
Copy link
Member Author

miri64 commented Jan 18, 2017

@OlegHahm Go home, you're drunk

Trollface

@smlng
Copy link
Member

smlng commented Jan 18, 2017

I tested with example/hello-world, and tried for instance -o and --stdout-pipe and same for stderr -> worked. What else do you suggest?

[edit] for the nit picky: on macOS, of course

@OlegHahm
Copy link
Member

Btw. the elf file grows by about 4kB.

@miri64
Copy link
Member Author

miri64 commented Jan 18, 2017

Just to test both long and short options, because during implementation I wasn't sure getopt_long() is actually working on non-GNU systems (though through research I found out it is, but still I have some sliver of doubt ;-)).

@miri64
Copy link
Member Author

miri64 commented Jan 18, 2017

Btw. the elf file grows by about 4kB.

I find more readable code and easier handling of command-line options actually better for native-only code, than thinking about code-size ;-)

@OlegHahm OlegHahm merged commit 7c04f05 into RIOT-OS:master Jan 18, 2017
@miri64 miri64 deleted the native/enh/pretty-startup branch January 18, 2017 22:18
@LudwigKnuepfer
Copy link
Member

Btw. the elf file grows by about 4kB.

That's about 5% for examples/gnrc_minimal and examples/hello-world, 2% for examples/gnrc_border_router.

More interesting (although probably as insignificant): what's the increase in RAM usage?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Platform: native Platform: This PR/issue effects the native platform 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