Skip to content

Conversation

@zhulipeng
Copy link
Member

@zhulipeng zhulipeng commented Apr 29, 2024

Delete unused declaration void *dictEntryMetadata(dictEntry *de); in dict.h.

@codecov
Copy link

codecov bot commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.38%. Comparing base (b0d5a0f) to head (fb8d5ef).
Report is 6 commits behind head on unstable.

❗ Current head fb8d5ef differs from pull request most recent head 0337939. Consider uploading reports for the commit 0337939 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #400      +/-   ##
============================================
+ Coverage     68.33%   68.38%   +0.04%     
============================================
  Files           108      108              
  Lines         61563    61563              
============================================
+ Hits          42072    42099      +27     
+ Misses        19491    19464      -27     

see 13 files with indirect coverage changes

Signed-off-by: Lipeng Zhu <[email protected]>
src/dict.h Outdated
Comment on lines 36 to 37
#ifndef DICT_H
#define DICT_H
Copy link
Member

Choose a reason for hiding this comment

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

Why rename this? It's a fairly common pattern in other code. I'm not sure if there is a strong reason to change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I remember @zuiderkwast requested such similar renaming because the leading double underscore which is a reserved name by the C standard.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mostly meant that when we're anyway renaming things like __REDIS_XYZ, we can drop the leading underscores at the same time.

I don't think we want do it all over the code base. (Sorry if I said that.)

Copy link
Member

Choose a reason for hiding this comment

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

What's the motivation to drop the leading underscores? It's just a little bit less consistent now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it will ever be a problem, unless name macros __ARM or something like that.

If you prefer to keep double underscores for consistency, I can accept that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lipzhu Please revert back to __DICT_H. If we ever want to change all these reserved names (for some kind of validator in the future) we can fix all files at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

If you prefer to keep double underscores for consistency, I can accept that.

I had read that reserved-names doc multiple times, and never noticed __ clause. I would prefer we keep what we have (since there isn't a conflict), but moving forward use the style that we mentioned here.

Copy link
Contributor

Choose a reason for hiding this comment

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

In case you still didn't find it:

In addition to the names documented in this manual, reserved names include all external identifiers (global functions and variables) that begin with an underscore (‘_’) and all identifiers regardless of use that begin with either two underscores or an underscore followed by a capital letter are reserved names.

@zuiderkwast zuiderkwast merged commit 44f273d into valkey-io:unstable May 1, 2024
@zhulipeng zhulipeng deleted the unused branch May 1, 2024 01:04
Signed-off-by: Lipeng Zhu <[email protected]>
PingXie pushed a commit to PingXie/valkey that referenced this pull request May 2, 2024
Delete unused declaration `void *dictEntryMetadata(dictEntry *de);` in
dict.h.

---------

Signed-off-by: Lipeng Zhu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants