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
Iox #1142 add service discovery c binding #1160
Iox #1142 add service discovery c binding #1160
Changes from all commits
54dbed9
7c7fe10
075f6b2
9a4f189
768a6f7
59e4326
2709b0c
d80c49a
f28d462
2a1fbe5
ba4e5c6
cdb5207
73df477
c9a4d43
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.
Appears fairly small (if it has space for 1024 Servicedescriptions each larger than 300 bytes). I am not sure something is off here. But will likely change (to something smaller) after #1151 because than we store those in dynamic memory.
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.
Got the numbers via "CI Debugging". Don't forget that you have to multiply by 8.
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.
Ah yes ... it is a uint64_t. Why I do not know (it is 8 byte aligned then, machine word on 64 bit architectures). But I am not sure access is always aligned for the actual type (many architectures can compensate this). Note that placement new requires an aligned pointer, which is not generally true for the ones we pass (
&do_not_touch_me
).TLDR: I think this is broken for some types (those with alignment greater than 8) but have not looked at the details (this is nothing particular to this PR). It is broken on machines that cannot enforce alignment on their own and even then the space may be insufficient.
If we do not have alignment > 8 types then it is ok, if we add those types in the future it can subtly break on some architectures.