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
C++ & Python API: add helpers for constructing an entity path #4595
C++ & Python API: add helpers for constructing an entity path #4595
Changes from 5 commits
1ca6636
1efc0f4
ef73085
bb67c24
38535a3
b14f7d2
e519509
2fd7fcb
2e02799
a5103b6
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.
suggestion:
instead of doing the alloc/free dance a slightly safer alternative would be to use the "buffer in, count inout" pattern:
In this fairly common C pattern the desired count is returned but the buffer isn't fully written. Meaning one can either pass a null for buffer to just figure out the size one needs to allocate, or speculatively put in a buffer and check whether a second call is needed.
The advantage is that in many cases only one call is needed and that allocation stays with the caller.
The disadvantage is that when passing null buffer or too small buffer initially, this bottoms out in more work being done.
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.
don't quite understand why
new_entity_path
is not fully implemented in the c interfaceThere 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 went with this implementation because it was much faster to implement. But it's also why I made it private.
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 understand the question.
new_entity_path
returns astd::string
, so cannot be defined in C.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.
you could pass in several
rr_string
and write to a single char* bufferSome generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 believe this should take a rerun collection. That way you can also call it with a single string view without overloading
Then again maybe that's a disadvantage because it muddles with the semantics of "list of path parts" 🤔
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.
There is plenty of room for improvement. I'd love to be able to call it with a mix of strings and integers to, and have the implementation call
to_string
on each non-string, but I feel this is good enough for now.