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

Refactor generic_data_handle. #2467

Closed
wants to merge 2 commits into from
Closed

Conversation

1uc
Copy link
Collaborator

@1uc 1uc commented Aug 18, 2023

The commit changes the generic_data_handle as follows:

  • Common sanitizing logic is consolidated in two methods assert_*.
  • The logic explicitly deals with the 'literal' and 'data_handle' cases separately.
  • Hides the type_name method, by making it private.
  • Adds assignment from a data_handle.
  • Since raw pointers are literal can_be_stored_literally allows pointers.
  • Adds tests demonstrating the ability to assign values (literally) to a non-null generic_data_handle in a type unsafe manner.
  • Improves documentation.

The commit changes the `generic_data_handle` as follows:

  * Common sanitizing logic is consolidated in two methods `assert_*`.
  * The logic explicitly deals with the 'literal' and 'data_handle'
    cases separately.
  * Hides the `type_name` method, by making it private.
  * Adds assignment from a `data_handle`.
  * Improves documentation.
@bbpbuildbot

This comment has been minimized.

@azure-pipelines
Copy link

✔️ 5dfa16c -> Azure artifacts URL

@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Merging #2467 (6025a9b) into master (efc28fe) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2467   +/-   ##
=======================================
  Coverage   60.41%   60.41%           
=======================================
  Files         626      626           
  Lines      120812   120823   +11     
=======================================
+ Hits        72990    73001   +11     
  Misses      47822    47822           
Files Changed Coverage Δ
src/neuron/container/generic_data_handle.hpp 100.00% <100.00%> (ø)
test/unit_tests/container/node.cpp 99.43% <100.00%> (+<0.01%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@1uc 1uc marked this pull request as ready for review August 18, 2023 18:21
@azure-pipelines
Copy link

✔️ 6025a9b -> Azure artifacts URL

* * small (`sizeof(T) <= sizeof(void*)`), trivial types such as `int` or `float`,
* * and raw pointers.
*
* In this context raw pointer is a regular C pointer, i.e. just and address.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* In this context raw pointer is a regular C pointer, i.e. just and address.
* In this context raw pointer is a regular C pointer, i.e. just an address.

test/unit_tests/container/node.cpp Show resolved Hide resolved
template <typename T>
static constexpr bool can_be_stored_literally_v =
std::is_trivial_v<T> && !std::is_pointer_v<T> && sizeof(T) <= sizeof(void*);
(std::is_trivial_v<T> && sizeof(T) <= sizeof(void*)) || std::is_pointer_v<T>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pointer types are trivial, so this is equivalent

Suggested change
(std::is_trivial_v<T> && sizeof(T) <= sizeof(void*)) || std::is_pointer_v<T>;
std::is_trivial_v<T> && sizeof(T) <= sizeof(void*);

but I am not totally convinced by the change to make can_be_stored_literally_v<double*> be true.

Storing small, trivial non-pointer values is an unfortunate feature supported by generic_data_handle that goes beyond it being a type-erased data_handle<T>. I hope that feature can be removed fairly promptly.

In contrast, storing a raw (in the sense that it is a not a permutation-stable handle to an entry in an soa container) T* is something that data_handle<T> does support, and which will not go away any time soon.

Given this, it seems preferable to keep as much of a distinction as possible between these two cases.

@1uc 1uc closed this Aug 23, 2023
@1uc 1uc deleted the 1uc/refactor-generic_data_handle branch October 19, 2023 15:27
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