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.

Sign up for GitHub

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

Jump to bottom

Improve type safety of key embedding #737

Closed
madolson opened this issue Jul 2, 2024 · 0 comments · Fixed by #749
Closed

Improve type safety of key embedding #737

madolson opened this issue Jul 2, 2024 · 0 comments · Fixed by #749
Assignees
@PingXie

Comments

@madolson
Copy link
Member

madolson commented Jul 2, 2024

Couple of things which remain

dict.c: In function ‘dictGetNextRef’:
dict.c:930:37: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
  930 |     if (entryIsEmbedded(de)) return &decodeEmbeddedEntry(de)->next;
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Yeah I did some experiments. It is actually not that big a change (and this is based on an older commit of @hpatro's PR). This really boils down to using dictEntry as an opaque structure as well and forcing access through common macros (SET/GET/GET_PTR) that perform common type translations. I think the changes are not that significant and if we could just do the first part (using dictEntry as an opaque structure) my concern will be addressed. Other refactoring can be addressed in future PRs.

Originally posted by @PingXie in #541 (comment)
@madolson madolson moved this to Todo in Valkey 8.0 Jul 2, 2024
@PingXie PingXie mentioned this issue Jul 5, 2024
Merged
@PingXie PingXie moved this from Todo to In Progress in Valkey 8.0 Jul 5, 2024
@madolson madolson moved this from In Progress to Optional for rc1 in Valkey 8.0 Jul 15, 2024
@madolson madolson moved this from Optional for rc2 to Todo in Valkey 8.0 Aug 5, 2024
PingXie added a commit that referenced this issue Sep 3, 2024
@PingXie @madolson
Improve type safety and refactor dict entry handling (#749)
981f977 
This pull request introduces several changes to improve the type safety
of Valkey's dictionary implementation:

- Getter/Setter Macros: Implemented macros `DICT_SET_VALUE` and
`DICT_GET_VALUE` to centralize type casting within these macros. This
change emulates the behavior of C++ templates in C, limiting type
casting to specific low-level operations and preventing it from being
spread across the codebase.

- Reduced Assert Overhead: Removed unnecessary asserts from critical hot
paths in the dictionary implementation.

- Consistent Naming: Standardized the naming of dictionary entry types.
For example, all dictionary entry types start their names with
`dictEntry`.


Fix #737

---------

Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@outlook.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
@github-project-automation github-project-automation bot moved this from In Progress to Done in Valkey 8.0 Sep 3, 2024
PingXie added a commit to PingXie/valkey that referenced this issue Sep 14, 2024
@PingXie @madolson
Improve type safety and refactor dict entry handling (valkey-io#749)
ab3b08a 
This pull request introduces several changes to improve the type safety
of Valkey's dictionary implementation:

- Getter/Setter Macros: Implemented macros `DICT_SET_VALUE` and
`DICT_GET_VALUE` to centralize type casting within these macros. This
change emulates the behavior of C++ templates in C, limiting type
casting to specific low-level operations and preventing it from being
spread across the codebase.

- Reduced Assert Overhead: Removed unnecessary asserts from critical hot
paths in the dictionary implementation.

- Consistent Naming: Standardized the naming of dictionary entry types.
For example, all dictionary entry types start their names with
`dictEntry`.

Fix valkey-io#737

---------

Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@outlook.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: Ping Xie <pingxie@google.com>
PingXie added a commit to PingXie/valkey that referenced this issue Sep 14, 2024
@PingXie @madolson
Improve type safety and refactor dict entry handling (valkey-io#749)
3a308b0 
This pull request introduces several changes to improve the type safety
of Valkey's dictionary implementation:

- Getter/Setter Macros: Implemented macros `DICT_SET_VALUE` and
`DICT_GET_VALUE` to centralize type casting within these macros. This
change emulates the behavior of C++ templates in C, limiting type
casting to specific low-level operations and preventing it from being
spread across the codebase.

- Reduced Assert Overhead: Removed unnecessary asserts from critical hot
paths in the dictionary implementation.

- Consistent Naming: Standardized the naming of dictionary entry types.
For example, all dictionary entry types start their names with
`dictEntry`.

Fix valkey-io#737

---------

Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@outlook.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: Ping Xie <pingxie@google.com>
PingXie added a commit to PingXie/valkey that referenced this issue Sep 14, 2024
@PingXie @madolson
Improve type safety and refactor dict entry handling (valkey-io#749)
2557237 
This pull request introduces several changes to improve the type safety
of Valkey's dictionary implementation:

- Getter/Setter Macros: Implemented macros `DICT_SET_VALUE` and
`DICT_GET_VALUE` to centralize type casting within these macros. This
change emulates the behavior of C++ templates in C, limiting type
casting to specific low-level operations and preventing it from being
spread across the codebase.

- Reduced Assert Overhead: Removed unnecessary asserts from critical hot
paths in the dictionary implementation.

- Consistent Naming: Standardized the naming of dictionary entry types.
For example, all dictionary entry types start their names with
`dictEntry`.

Fix valkey-io#737

---------

Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@outlook.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: Ping Xie <pingxie@google.com>
enjoy-binbin pushed a commit to enjoy-binbin/valkey that referenced this issue Sep 14, 2024
@PingXie @madolson @enjoy-binbin
Improve type safety and refactor dict entry handling (valkey-io#749)
cc01b6d 
This pull request introduces several changes to improve the type safety
of Valkey's dictionary implementation:

- Getter/Setter Macros: Implemented macros `DICT_SET_VALUE` and
`DICT_GET_VALUE` to centralize type casting within these macros. This
change emulates the behavior of C++ templates in C, limiting type
casting to specific low-level operations and preventing it from being
spread across the codebase.

- Reduced Assert Overhead: Removed unnecessary asserts from critical hot
paths in the dictionary implementation.

- Consistent Naming: Standardized the naming of dictionary entry types.
For example, all dictionary entry types start their names with
`dictEntry`.


Fix valkey-io#737

---------

Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@outlook.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
PingXie added a commit to PingXie/valkey that referenced this issue Sep 14, 2024
@PingXie @madolson
Improve type safety and refactor dict entry handling (valkey-io#749)
5cdd78d 
This pull request introduces several changes to improve the type safety
of Valkey's dictionary implementation:

- Getter/Setter Macros: Implemented macros `DICT_SET_VALUE` and
`DICT_GET_VALUE` to centralize type casting within these macros. This
change emulates the behavior of C++ templates in C, limiting type
casting to specific low-level operations and preventing it from being
spread across the codebase.

- Reduced Assert Overhead: Removed unnecessary asserts from critical hot
paths in the dictionary implementation.

- Consistent Naming: Standardized the naming of dictionary entry types.
For example, all dictionary entry types start their names with
`dictEntry`.

Fix valkey-io#737

---------

Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@outlook.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: Ping Xie <pingxie@google.com>
PingXie added a commit to PingXie/valkey that referenced this issue Sep 14, 2024
@PingXie @madolson
Improve type safety and refactor dict entry handling (valkey-io#749)
744a9c6 
This pull request introduces several changes to improve the type safety
of Valkey's dictionary implementation:

- Getter/Setter Macros: Implemented macros `DICT_SET_VALUE` and
`DICT_GET_VALUE` to centralize type casting within these macros. This
change emulates the behavior of C++ templates in C, limiting type
casting to specific low-level operations and preventing it from being
spread across the codebase.

- Reduced Assert Overhead: Removed unnecessary asserts from critical hot
paths in the dictionary implementation.

- Consistent Naming: Standardized the naming of dictionary entry types.
For example, all dictionary entry types start their names with
`dictEntry`.

Fix valkey-io#737

---------

Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@outlook.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: Ping Xie <pingxie@google.com>
PingXie added a commit that referenced this issue Sep 15, 2024
@PingXie @madolson
Improve type safety and refactor dict entry handling (#749)
6f2b3d4 
This pull request introduces several changes to improve the type safety
of Valkey's dictionary implementation:

- Getter/Setter Macros: Implemented macros `DICT_SET_VALUE` and
`DICT_GET_VALUE` to centralize type casting within these macros. This
change emulates the behavior of C++ templates in C, limiting type
casting to specific low-level operations and preventing it from being
spread across the codebase.

- Reduced Assert Overhead: Removed unnecessary asserts from critical hot
paths in the dictionary implementation.

- Consistent Naming: Standardized the naming of dictionary entry types.
For example, all dictionary entry types start their names with
`dictEntry`.

Fix #737

---------

Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@outlook.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: Ping Xie <pingxie@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@PingXie PingXie

Labels
None yet
Projects
Valkey 8.0
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve type safety and refactor dict entry handling PingXie/valkey
2 participants
@PingXie @madolson