search

sorear / smetamath-rs

sorear's Metamath system engine - version 3 Rust
Apache License 2.0
22 stars 6 forks source link

add Deref impls for *Ref structs #23

Closed digama0 closed 8 years ago

digama0 commented 8 years ago

StatementRef was left off because Statement is not public.

sorear commented 8 years ago

OK for now. FYI, eventually I'd kinda like to get rid of Deref for SegmentRef and make SegmentRef.segment private; nobody outside parser and segment_set should know that Segment objects exist

digama0 commented 8 years ago

What's wrong with having Segment public with all private fields, and move the applicable API functions to Segment? Same for Statement(Ref).

sorear commented 8 years ago

The main argument was fragmentation; since most users will have a SegmentRef, they shouldn't have to think about whether the API they want is handled on SegmentRef or Segment. Although I suppose that argument goes away with a Deref impl.

Statement is a different case because there's very little you can do with a Statement if you don't also have references to the buffer and the span_pool. Since most of the API has to be on StatementRef anyway, and there's nobody outside parser that needs to handle raw Statements, I'd rather keep Statement as it is right now.

sorear commented 8 years ago

Er, this doesn't actually build, so I had to roll it back (where's the "reopen" button?)

sorear commented 8 years ago

Tried fixing the type error, discovered that this is actually a non-starter because the lifetime of Deref is wrong. Deref does not allow you to return a slice that potentially outlives the "smart reference"

digama0 commented 8 years ago

@sorear Oops, forgot to build :/ I was actually aware of this issue; about half of the uses of TokenRef::slice or SegmentRef::segment are not replaceable with derefs because of lifetime constraints. Nevertheless, a lot of the time you just need some lifetime, in order to extract a Copy value, and in these cases the deref is still useful.