This repository has been archived by the owner on Jun 28, 2022. It is now read-only.
Return SerializeResult for serializing a profile #42
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ivoanjo
approved these changes
Mar 30, 2022
Cargo.lock
Outdated
Comment on lines
106
to
109
| [[package]] | ||
| name = "ddprof" | ||
| version = "0.4.0-rc.1" | ||
| version = "0.5.0-rc.1" | ||
| dependencies = [ |
There was a problem hiding this comment.
Minor: I already merged this change to master in #39 so I think this is going to conflict.
Comment on lines
+29
to
+30
| #[export_name = "ddprof_ffi_NewProfileExporterV3Result_drop"] | ||
| pub unsafe extern "C" fn new_profile_exporter_v3_result_drop(result: NewProfileExporterV3Result) { |
There was a problem hiding this comment.
Minor: As we're going to rename this for consistency anyway (thanks!) may I suggest calling it ddprof_ffi_ProfileExporterV3Result_drop?
Indeed the type is called NewProfileExporterV3Result but that's a bit of a mouthful and we could rename it later as well :) (probably not in this PR, to avoid getting too off-topic). I actually think that since we're not too worried about backwards-compatibility right now we could even just rename it back to ProfileExporter without New or V3 suffixes.
There was a problem hiding this comment.
I'll leave it for now -- we can shorten and clean this up later (like next PR, possibly).
morrisonlevi
force-pushed
the
levi/vec
branch
2 times, most recently
from
2e39741
to
76aad26
Compare
The Vec type is a generalization of Buffer. In particular, this PR exposes an API to create and manage Vec<Tag> and replaces existing uses of Buffer with Vec<u8>. It includes a helper to parse DD_TAGS as well. One of the APIs, ddprof_ffi_Buffer_reset, was removed. Previously this was required when using the failure case of the SendResult type, but now there is ddprof_ffi_SendResult_drop, which should be used instead.
morrisonlevi
force-pushed
the
levi/serialize
branch
from
1183de2
to
7c0ed24
Compare
Previously it couldn't handle a tags like: env:staging:east value Tag messages on failure have improved. As part of adding tests, added more trait support to Slice and CharSlice so they can be used in equivalence statements. As part of doing that, I realized Slice could implement IntoIterator and that it cleaned up 3+ places where we call .into_slice().iter(). I also removed unsafe from `into_slice`. Technically, it could still fail such as if the user creates a slice of length 4 but only has 2 elements, but this is a user error that cannot really be checked.
morrisonlevi
force-pushed
the
levi/serialize
branch
from
7c0ed24
to
3c4e4eb
Compare
This removes the direct `eprintln` usage and allows the caller to choose what to do with it instead. Adds these FFI functions: - `ddprof_ffi_SerializeResult_drop` - `ddprof_ffi_Vec_u8_as_slice`. The EncodedProfile has a Vec<u8> but the struct ddprof_ffi_File requires a Slice<u8>. And removes `ddprof_ffi_EncodedProfile_delete`.
morrisonlevi
force-pushed
the
levi/serialize
branch
from
3c4e4eb
to
f7dd75b
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
What does this PR do?
Changes:
ddprof_ffi_Profile_serializereturnsddprof_ffi_SerializeResultAdds:
ddprof_ffi_SerializeResult_dropddprof_ffi_Vec_u8_as_sliceRemoves
ddprof_ffi_EncodedProfile_delete.Cleans up exporter example. With the newer APIs, fewer helpers are
needed.
Motivation
Ivo wrote a doc for API changes he wants and I had a moment and was
already In The Groove. These changes make it easier for the caller to
choose what to do with errors, and make it easier to convert the
EncodeProfile's buffer field into a slice.
Additional Notes
I like working with Ivo.
How to test the change?
Observe that existing code breaks, adjust code paths around the above
changes, and be on your way. I fixed up the exporter example so you can
refer to that too.