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

optparse: fix incorrect error message when invalid short option is grouped with valid short options #1183

Merged
merged 2 commits into from Sep 9, 2017

Conversation

Projects
None yet
4 participants
@grondo
Copy link
Contributor

grondo commented Sep 7, 2017

As described in #1179, optparse error message for unrecognized option is not correct when an invalid short option is used and grouped with one or more valid options.

Fix this by assuming an invalid short option when optopt != '\0' and use the char in optopt as the unrecognized option in the error output. Otherwise, assume a long option and use argv [optind-1] as the invalid option to display.

Simple tests were also added to ensure valid error messages for unrecognized short options, grouped and ungrouped.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 7, 2017

Coverage Status

Coverage increased (+0.04%) to 78.203% when pulling 71a46ad on grondo:issue#1179 into 9c860d0 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 8, 2017

This error is popping up in travis, in unrelated code. Not sure what to make of it.

make[4]: Entering directory `/home/travis/build/flux-framework/flux-core/flux-core-0.8.0-88-gb9d062d2/_build/sub/src/common/libsubprocess'
  CC       zio.lo
  CC       subprocess.lo
../../../../../src/common/libsubprocess/subprocess.c:1092:19: error: passing an
      object that undergoes default argument promotion to 'va_start' has
      undefined behavior [-Werror,-Wvarargs]
    va_start (ap, item);
                  ^
../../../../../src/common/libsubprocess/subprocess.c:1085:66: note: parameter of
      type 'sm_item_t' (aka 'enum sm_item') is declared here
subprocess_manager_set (struct subprocess_manager *sm, sm_item_t item, ...)
@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Sep 8, 2017

they did just update the trusty images recently, so perhaps a new warning in a small compiler update?

Actually this is a real error, apparently passing any value with a default type promotion before the ellipsis in a variadic function is a no-no. E.g. in this case the enum is promoted to an integer type and I guess isn't exactly defined what the size of the type would be.

Will have to find a quick fix.

https://www.securecoding.cert.org/confluence/display/cplusplus/EXP58-CPP.+Pass+an+object+of+the+correct+type+to+va_start

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 8, 2017

Codecov Report

Merging #1183 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1183      +/-   ##
==========================================
- Coverage   77.82%   77.78%   -0.04%     
==========================================
  Files         158      158              
  Lines       29255    29257       +2     
==========================================
- Hits        22768    22758      -10     
- Misses       6487     6499      +12
Impacted Files Coverage Δ
src/common/liboptparse/optparse.c 88.48% <100%> (+0.03%) ⬆️
src/common/libflux/mrpc.c 85.15% <0%> (-1.18%) ⬇️
src/common/libkvs/kvs_watch.c 86.34% <0%> (-0.89%) ⬇️
src/common/libflux/future.c 88.61% <0%> (-0.5%) ⬇️
src/common/libflux/message.c 80.94% <0%> (-0.36%) ⬇️
src/broker/overlay.c 71.67% <0%> (-0.35%) ⬇️
src/broker/module.c 83.28% <0%> (-0.28%) ⬇️
src/modules/kvs/kvs.c 63.16% <0%> (-0.27%) ⬇️
src/common/libkvs/kvs.c 65.08% <0%> (-0.26%) ⬇️
src/common/libflux/response.c 84.55% <0%> (ø) ⬆️
... and 4 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 8, 2017

Coverage Status

Coverage increased (+0.03%) to 78.192% when pulling badc987 on grondo:issue#1179 into 9c860d0 on flux-framework:master.

@grondo grondo force-pushed the grondo:issue#1179 branch from badc987 to ec2011a Sep 8, 2017

grondo added some commits Sep 7, 2017

optparse/test: add test for bad short option error messages
Add test that the correct unrecognized short option is displayed
in the optparse error messages when getopt calls out unknown option.
optparse: emit correct unrecognized option character
Emit the correct unrecognized option character on error when
short options are used and grouped on the commandline.

Fixes #1179.

@grondo grondo force-pushed the grondo:issue#1179 branch from ec2011a to aed7f57 Sep 8, 2017

@grondo grondo changed the title optparse: fix invalid error message when invaled short option is grouped with valid short options optparse: fix incorrect error message when invalid short option is grouped with valid short options Sep 8, 2017

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Sep 8, 2017

Rebased on current master, so we should make it through travis this time.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 8, 2017

Coverage Status

Coverage decreased (-0.03%) to 78.135% when pulling aed7f57 on grondo:issue#1179 into ea1f316 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Sep 8, 2017

One build hit a variant of #1169. Restarting.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 9, 2017

Thanks!

@garlick garlick merged commit 8443eff into flux-framework:master Sep 9, 2017

4 checks passed

codecov/patch 100% of diff hit (target 77.82%)
Details
codecov/project Absolute coverage decreased by -0.03% but relative coverage increased by +22.17% compared to ea1f316
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 78.135%
Details

@grondo grondo deleted the grondo:issue#1179 branch Oct 25, 2017

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

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.