add support for traceable collections to Rust#6384
Conversation
|
LGTM |
ff76bff to
05b4eea
Compare
guybedford
left a comment
There was a problem hiding this comment.
I'm sorry I just don't think this is the right approach - matching name strings is clearly in violation of scoping rules, as I've mentioned before.
Please explain to me why we aren't using a trait to handle this?
5580e83 to
974acac
Compare
|
Addressed the review feedback:
Could you please take another look? |
jasnell
left a comment
There was a problem hiding this comment.
Findings from review:
- [MEDIUM]
name+custom_tracecombination silently drops the name override —extract_name_attributeparses the full attr stream as a singleMetaNameValue, which fails whencustom_traceis also present - [MEDIUM]
has_custom_trace_flaguses string-based parsing rather thansynmeta parsing, which is inconsistent with the rest of the crate - [LOW]
RefCell<T>tracing usesself.borrow()which will panic if a mutable borrow is active during GC
This review was generated by an AI assistant and may contain inaccuracies.
guybedford
left a comment
There was a problem hiding this comment.
Please can we just add one more thing to this PR - a derive macro for Traced on structs and enums?
Example:
#[derive(jsg::Traced)]
struct MyPlainData { ... }
#[derive(jsg::Traced)]
enum MyPlainData { ... }
basically works just like the current trace.rs does anyway.
guybedford
left a comment
There was a problem hiding this comment.
Marking this as conditional approval on the comments and derive macro being added.
974acac to
282b449
Compare
282b449 to
2b8ac69
Compare
Addresses the following comments by @jasnell: