-
Notifications
You must be signed in to change notification settings - Fork 22
C++ runtime and hyperobjects #58
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
Conversation
runtime/local-reducer-api.cpp
Outdated
| { | ||
| struct bucket b { | ||
| .key = (uintptr_t)key, | ||
| .hash = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, hash is always initialized to 0 in a new bucket. Why not specify that default initialization in the definition of bucket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value in the struct definition is 0. I think I didn't want to mix aggregate and constructor style initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, the particular mixing of aggregate and default initialization that would happen here, if this line didn't initialize .hash explicitly, has well-defined behavior as of C++11.
I think it's actually cleaner to rely on the default initialization of .hash in places like this, rather than explicitly initialize it. In my mind, the default initialization of .hash encodes the correct value of .hash for any new bucket, which is dictated by the hypertable structure and not the call to the registration/lookup function itself. Letting the default initialization handle .hash allows for less code duplication and less error-prone code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the explicit initialization.
| struct bucket b { | ||
| .key = (uintptr_t)key, | ||
| .hash = 0, | ||
| .data = { .view = nullptr, .extra = key } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other reducer_register functions all set view to the key. Why is this one different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the 0-argument variant lookup calls return extra as a reducer_base * and ignore view. In the 1- and 2-argument variants lookup calls return view and ignore extra. This difference is to allow reducer_base to be a virtual base class of the view type.
9fcd612 to
5ae7925
Compare
include/cilk/reducer
Outdated
| struct __reducer_base { | ||
| __reducer_base(); | ||
| virtual ~__reducer_base(); | ||
| virtual std::size_t size() = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the size() function be const? Is there a use case where this needs to not be const? If not, then it seems better to make this const and prevent worries about needing to synchronize concurrent calls to this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In C++ a const method is still allowed to modify state. If you want size() to be const for documentation purposes ("we prefer that the method not have side effects") I can make it const. Only the runtime should be calling size().
5ae7925 to
5be3c08
Compare
0724d85 to
83a5abf
Compare
Convert runtime to C++. Create two new reducer kinds for C++ code.
This change must be paired with the corresponding compiler change.