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

Rename all ctx_t structs to make them unique. #978

Merged
merged 1 commit into from Feb 17, 2017

Conversation

Projects
None yet
5 participants
@morrone
Copy link
Contributor

morrone commented Feb 10, 2017

Rename all ctx_t structs to give them a unique name. While they are
ll used in ways that avoid conflict (usually local to a single file),
it makes it more difficult for the new developer to navigate through
the code when many different structs share the same name (because
ools like cscope and ctags will show many definition locations for
the struct rather than showing the one correct location).

Fixes #972

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 10, 2017

Maybe distill this commit message down to one liner for RFC 7 so new and old developers don't inadvertently recreate this situation?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 10, 2017

Nice thanks!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 10, 2017

Coverage Status

Coverage decreased (-0.01%) to 76.171% when pulling 164bb3c on morrone:rename_ctx_t into edc914e on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 10, 2017

Codecov Report

Merging #978 into master will increase coverage by <.01%.
The diff coverage is 85.08%.

@@            Coverage Diff             @@
##           master     #978      +/-   ##
==========================================
+ Coverage   75.92%   75.93%   +<.01%     
==========================================
  Files         152      152              
  Lines       25936    25936              
==========================================
+ Hits        19693    19695       +2     
+ Misses       6243     6241       -2
Impacted Files Coverage Δ
src/broker/modservice.c 64.03% <100%> (+0.87%)
src/common/libflux/attr.c 94.5% <100%> (ø)
src/modules/barrier/libbarrier.c 100% <100%> (ø)
src/connectors/local/local.c 87.87% <100%> (ø)
src/connectors/loop/loop.c 86.2% <100%> (ø)
src/modules/live/live.c 51.34% <57.57%> (ø)
src/modules/content-sqlite/content-sqlite.c 54.84% <81.81%> (ø)
src/connectors/ssh/ssh.c 75.23% <84.61%> (ø)
src/cmd/builtin/proxy.c 72.6% <85.71%> (ø)
src/modules/kvs/kvs.c 80.58% <86.04%> (-0.13%)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b877c17...caee41c. Read the comment docs.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 10, 2017

Maybe distill this commit message down to one liner for RFC 7 so new and old developers don't inadvertently recreate this situation?

Can I suggest

Type names SHOULD be chosen such that they are unique within a framework project. For example avoid generic types like struct a or typedefs like ctx_t. A good practice is to include the name of the relevant source file, module or service in the typedef (like fooservice_ctx_t).

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 16, 2017

The travis failure looks like it might have been something on the travis end?
This needs to be rebased on current master then it should go in. Thanks Chris.

Rename all ctx_t structs to make them unique.
Rename all ctx_t structs to give them a unique name.   While they are
ll used in ways that avoid conflict (usually local to a single file),
it makes it more difficult for the new developer to navigate through
the code when many different structs share the same name (because
ools like cscope and ctags will show many definition locations for
the struct rather than showing the one correct location).

Fixes #972
@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Feb 17, 2017

Rebased.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 17, 2017

Coverage Status

Coverage increased (+0.007%) to 76.211% when pulling caee41c on morrone:rename_ctx_t into b877c17 on flux-framework:master.

@garlick garlick merged commit 9074ac6 into flux-framework:master Feb 17, 2017

4 checks passed

codecov/patch 85.08% of diff hit (target 75.92%)
Details
codecov/project 75.93% (+<.01%) compared to b877c17
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 76.211%
Details

@morrone morrone deleted the morrone:rename_ctx_t branch Feb 17, 2017

@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.