Conversation
| @@ -0,0 +1,30 @@ | |||
| #pragma once | |||
|
|
|||
| #include <boost/uuid/uuid.hpp> | |||
There was a problem hiding this comment.
I tried to forward declare the type I needed here but couldn't figure out how.
There was a problem hiding this comment.
We need to store a value of this type in our struct, so we need the definition. We can only get away with a forward decl when we're dealing with only refs and pointers, since they're always the same size.
There was a problem hiding this comment.
Note that boost has been removed as a dependency. For this, we can use stduuid: https://github.com/mariusbancila/stduuid
We'll need to bring it in as a submodule, under compiler+runtime/third-party.
| processor::object_result processor::parse_reader_macro_tagged() | ||
| { | ||
| auto const token((*token_current).expect_ok()); | ||
| auto const tag(boost::get<native_persistent_string_view>(token.data)); |
There was a problem hiding this comment.
I really struggled to get the tag here, not sure if I did it correctly.
| return o->type == object_type::tagged_literal; | ||
| } | ||
|
|
||
| object_ptr parse_uuid(object_ptr const o) |
There was a problem hiding this comment.
parse-uuid on the jvm is odd in that it will throw if the input value is the wrong type, but will return nil if the string is invalid. However, using #uuid "invalid-string" will throw an exception. Not sure how much of the exception behavior we want to mirror of the jvm, but I tried to stay close.
|
|
||
| object base{ obj_type }; | ||
|
|
||
| boost::uuids::uuid value{}; |
There was a problem hiding this comment.
I went with this representation as I assume boost has some internal compact representation, however I did not verify. Also, not sure what to call this field.
There was a problem hiding this comment.
Actually I can just goto definition 🤦
using repr_type = std::uint8_t[ 16 ];There was a problem hiding this comment.
This is just fine. If the uuid header shows up as a heft include, we can later change the struct to store something opaque and only include boost's uuid in the cpp file. At this point, I don't think we can do that without affecting runtime perf.
jeaye
left a comment
jeaye
left a comment
There was a problem hiding this comment.
Looks great. Some minor comments. The only major comment is this: you're missing something. :)
Please add a test which creates an anonymous fn which returns a uuid and calls the fn immediately. You'll find it fails. The reason is that we need IR gen for this new object, since we want to carry it into IR land.
To do that, we need to:
- Add a new uuid fn to
c-api.h - Add a
gen_globaloverload foruuid_ptr - Verify your test passes
| @@ -0,0 +1,3 @@ | |||
| (assert (uuid? (random-uuid))) | |||
There was a problem hiding this comment.
The tests here are only for the compiler, not the runtime. We have a whole other repo of tests for the runtime (clojure.core-test). We can remove this test and change the one above it to not check uuid?. I think we can have more negative cases, though, such as:
- Empty string
- Valid UUID string with extra characters at the end
| return s->value == value; | ||
| } | ||
|
|
||
| static void to_string_impl(boost::uuids::uuid value, util::string_builder &buff) |
There was a problem hiding this comment.
boost::uuids::uuid is a struct type, so please pass it by const & to avoid copying.
|
|
||
| object base{ obj_type }; | ||
|
|
||
| boost::uuids::uuid value{}; |
There was a problem hiding this comment.
This is just fine. If the uuid header shows up as a heft include, we can later change the struct to store something opaque and only include boost's uuid in the cpp file. At this point, I don't think we can do that without affecting runtime perf.
| @@ -0,0 +1,30 @@ | |||
| #pragma once | |||
|
|
|||
| #include <boost/uuid/uuid.hpp> | |||
There was a problem hiding this comment.
We need to store a value of this type in our struct, so we need the definition. We can only get away with a forward decl when we're dealing with only refs and pointers, since they're always the same size.
| boost::uuids::uuid value{}; | ||
|
|
||
| mutable native_hash hash{}; | ||
| }; |
There was a problem hiding this comment.
Every object foo needs a foo_ptr alias before it. Check with the other object types for how it works.
|
@jeaye I think a problem with #193 (review) is that the reader is open, users can provide their own fns and return any object they want. In those instances, how would it work if the type isn't part of the the |
|
@djblue Any interest in pushing this across the finish line, or are you done with it? |
|
Hi @jeaye, I don't think I can get this over the line. I think the scope of this PR is a little larger than I have bandwidth for currently, sorry. |
All good, my dude. Thanks for getting it this far! I'll make sure it gets finished. |
2 participants
No description provided.