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

kvs: [cleanup] fix blobref_t typedef and unfortunate variable names #1279

Merged
merged 5 commits into from Nov 8, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Nov 8, 2017

This is mainly cosmetic. As noted in #1222 the use of the "hp" variable name to refer to an internal KVS cache entry is ubiquitous and confusing. Change it to "entry".

Drop the href_t typedef in the KVS and create an identical definition called blobref_t over in libutil/blobref.h. Change some of the blobref functions to accept the fixed length char array rather than char pointer and length to make less verbose to use by its primary user in the KVS.

Drop the treeobj_hash() function which no longer has any users (replicating it in unit tests that need it, and changing it to use a blobref_t argument).

garlick added some commits Nov 8, 2017

libutil/blobref: define blobref_t type
Define a fixed length array that can hold any blobref.

Use it in blobref unit test.
modules/kvs: replace href_t with blobref_t
Eliminate href_t typedef and switch users to blobref_t.

Drop types.h which only contained that one typedef.
libkvs/treeobj: drop treeobj_hash()
Problem: treeobj_hash() no longer has users, except in tests.

Drop the function from the treeobj.c and replicate it in the
KVS lookup and commit unit tests.

Update treeobj unit tests.
libutil/blobref: used fixed length string param
Use blobref_t, a fixed length string type, as the blobref param to
  blobref_hash()
  blobref_hashtostr()
  blobref_strtohash()
and drop the size parameter since the size is implicit in the type.

Update various users and tests.
modules/kvs: [cleanup] rename cache entry variable
Problem: the variable "hp is used extensively to refer to
an internal KVS cache entry, which is not intuitive.

Change all occurrences of the variable "hp" to "entry",
when it refers to a cache entry.

Fixes #1222
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 8, 2017

Coverage Status

Coverage decreased (-0.008%) to 78.543% when pulling 72b930c on garlick:blobref_t into b10c1c7 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Nov 8, 2017

Nothing stood out to me. I'm glad to see href_t out of modules/kvs now. Can hit merge when it turns green.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 8, 2017

Thanks - I'm good with merging this if you are.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 8, 2017

Codecov Report

Merging #1279 into master will decrease coverage by <.01%.
The diff coverage is 93.08%.

@@            Coverage Diff             @@
##           master    #1279      +/-   ##
==========================================
- Coverage   77.95%   77.95%   -0.01%     
==========================================
  Files         154      154              
  Lines       29097    29086      -11     
==========================================
- Hits        22684    22675       -9     
+ Misses       6413     6411       -2
Impacted Files Coverage Δ
src/common/libutil/blobref.c 98.61% <100%> (ø) ⬆️
src/modules/kvs/lookup.c 80% <100%> (ø) ⬆️
src/modules/content-sqlite/content-sqlite.c 56.75% <100%> (ø) ⬆️
src/common/libkvs/treeobj.c 85.54% <100%> (-0.67%) ⬇️
src/broker/content-cache.c 73.43% <100%> (-0.65%) ⬇️
src/modules/kvs/commit.c 75.77% <85.71%> (ø) ⬆️
src/modules/kvs/kvs.c 64.01% <87.17%> (+0.25%) ⬆️
src/modules/kvs/cache.c 90.5% <94.94%> (+0.05%) ⬆️
src/common/libflux/mrpc.c 85.49% <0%> (-1.18%) ⬇️
src/broker/modservice.c 80.58% <0%> (-0.98%) ⬇️
... and 11 more

@chu11 chu11 merged commit cc5cd0a into flux-framework:master Nov 8, 2017

4 checks passed

codecov/patch 93.08% of diff hit (target 77.95%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +15.12% compared to b10c1c7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.008%) to 78.543%
Details

@garlick garlick deleted the garlick:blobref_t branch Nov 10, 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.