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

Nitpick cleanup of liboptparse usage #922

Merged
merged 2 commits into from Dec 13, 2016

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Dec 13, 2016

This is a nitpick patch. I'm not going to fight too hard over it if it's disliked.

I noticed in a number of places optind was used as a variable name when parsing args with liboptparse. optind is a global variable used by getopt(3). Sometimes the variable was used without declaration, as code just used the global one from getopt.

To try and disentangle the two parsing libraries I renamed variable usage everywhere, and declared variables when appropriate.

I then (perhaps going too far) renamed optparse_optind() to optparse_option_index(). I felt the function name invited people to use optind as a variable name.

@garlick garlick added the review label Dec 13, 2016

@chu11 chu11 force-pushed the chu11:optindcleanup branch from 073a193 to 2b1065a Dec 13, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 13, 2016

Current coverage is 76.00% (diff: 90.00%)

Merging #922 into master will increase coverage by <.01%

@@             master       #922   diff @@
==========================================
  Files           149        149          
  Lines         26001      26001          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          19760      19761     +1   
+ Misses         6241       6240     -1   
  Partials          0          0          
Diff Coverage File Path
0% src/cmd/builtin/heaptrace.c
0% src/cmd/builtin/nodeset.c
•••••••••• 100% src/cmd/builtin/content.c
•••••••••• 100% src/cmd/flux-event.c
•••••••••• 100% src/cmd/builtin/attr.c
•••••••••• 100% src/cmd/builtin/help.c
•••••••••• 100% src/cmd/flux-start.c
•••••••••• 100% src/cmd/builtin/env.c
•••••••••• 100% src/cmd/flux-ping.c
•••••••••• 100% src/cmd/flux.c

Review all 14 files changed

Powered by Codecov. Last update 6ac06fa...0742a2c

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 13, 2016

Coverage Status

Coverage decreased (-0.01%) to 76.297% when pulling 2b1065a on chu11:optindcleanup into 99a6800 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 13, 2016

Coverage Status

Coverage decreased (-0.03%) to 76.282% when pulling 2b1065a on chu11:optindcleanup into 99a6800 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 13, 2016

Are we mixing getopt(3) and liboptparse usage somewhere where the use of a local optind caused an error or warning? liboptparse is now using its own internal copy of getopt_long_r(), which doesn't call getopt(3) so the optarg, optind, optopt, etc globals should not be used at all (I thought), and code using liboptparse shouldn't really include getopt.h so those extern declarations should not be in scope either.

If it does bother you, I'm fine with rename of optind locals, but I don't think we could protect against this from all callers, and does it kind of set of precedent that we have to rename all variables that might be a global in some unrelated context, or from headers not included in the current module?

I'll go along with whatever group consensus is here, but I have to admit I'm having trouble with the amount of code churn this small change causes. (Worth it if it increases clarity or quiets errors, though)

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 13, 2016

@chu11 -- Oh, I do notice now how you did remove #include <getopt.h> in a few places and where you fixed up inadvertent uses of the optind global. These are good fixups! I can see now how you ended up here...

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 13, 2016

@grondo No, there weren't any errors. I just noticed optind being used in some places, but it not being declared anywhere. It confused me until I realized how it was working. I copied the bad use of optind from flux.c into flux-ping.c, so that's partially why I decided to clean this up. Perhaps I went too far with the second patch where I renamed a function. If you think it's too much churn, I could just axe that second patch.

To my surprise, the removal of #include <getopt.h> didn't cause an error in some cases because (to my surprise) unistd.h happens to include getopt.h.

Yeah, so this is totally nit-picky.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 13, 2016

To my surprise, the removal of #include <getopt.h> didn't cause an error in some cases because (to my surprise) unistd.h happens to include getopt.h.

@chu11, yeah your changes totally make sense to me once I looked through them. I'm actually now thinking renaming of optparse_optind is also a good idea. The ability to inadvertently use global optind is just a little too disturbing (I didn't know getopt.h was included with unistd.h either)

Thanks!

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 13, 2016

Yeah, that was my feeling too. The accidental use of the global felt wrong.

Appears I need to rebase. I'll push an updated branch up.

chu11 added some commits Dec 13, 2016

liboptparse: Remove use of 'optind' variable
To disentangle the legacy of the 'optind' global variable from
getopt() with the liboptparse library, rename all uses of 'optind'
variable to another variable name.  Declare variable when necessary.
liboptparse: Rename optparse_optind()
To disentangle the legacy of the 'optind' global variable from
getopt() within the liboptparse library, rename all use of the
function optparse_optind() to optparse_option_index().  Rename
all internal use of 'optind' variables to 'option_index'.

@chu11 chu11 force-pushed the chu11:optindcleanup branch from 2b1065a to 0742a2c Dec 13, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 13, 2016

Coverage Status

Coverage increased (+0.007%) to 76.309% when pulling 0742a2c on chu11:optindcleanup into 6ac06fa on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 13, 2016

Looks good, thanks!

@grondo grondo merged commit e84a5b5 into flux-framework:master Dec 13, 2016

4 checks passed

codecov/patch 90.00% of diff hit (target 75.99%)
Details
codecov/project 76.00% (+<.01%) compared to 6ac06fa
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 76.309%
Details

@grondo grondo removed the review label Dec 13, 2016

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.