Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
vine: fixed_location task groups #3787
vine: fixed_location task groups #3787
Changes from 19 commits
780ca03
3daddab
76faf91
e73e306
0461540
874f972
523c965
3f5a85c
9785d93
2611c7f
b400b84
f76e7cd
07ecee9
0eea277
c156c8e
715fa81
c4d5283
4537cb1
587eb35
2bc861f
d1945f7
673ae06
b7c20b2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Does it need
task->group_id = NULL
? inside this if?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 don't think so since the task is getting deleted afterwards
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'd prefer to have the
if (task->group_id) {
only, set it to NULL inside the if, and do not check for therefcount
. The only place where we should be checking for the refcount is when deleting the task, otherwise it gets tricky if we ever call functions in a different context.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.
why is group_id a string? Let's keep an int if possible so there is less chance of memory leaks/segfaults.
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.
it is because the group_id is a key to a hash table. They used to be a full uuid string but that was deemed to be excessive since the number of groups does not get very large.
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 suggest using
struct itable
then. It is likestruct hash_table
but the keys are ints.