search

gimli-rs / gimli

A library for reading and writing the DWARF debugging format
https://docs.rs/gimli/
Apache License 2.0
853 stars 108 forks source link

Reader::to_{slice,string} return GATs #682

Closed tamird closed 1 year ago

tamird commented 1 year ago

Currently the API implies that whether the return is owned or borrowed can vary per call, but in fact a given reader type always returns owned or borrowed values.

Increase the MSRV for the read feature to 1.65.0 which stabilized generic associated types.

tamird commented 1 year ago

Ah I misread https://github.com/gimli-rs/gimli/commit/39478799020819314bdf7aac8cc3e08fe451269b - the MSRV for the read feature is still 1.60.0. This requires that to be bumped, updated the description and CI configs.

philipc commented 1 year ago

A few things to note:

Given the above, I don't see the benefit of making this change. Can you give your motivation for this? This is the second time you have wanted to change these methods in some way.

tamird commented 1 year ago

A few things to note:

  • this is a breaking change

Agreed.

  • to_slice intentionally returns a Result in case a reader implementation requires that

That's a point, and can certainly be reverted.

  • the change from Cow to a GAT decreases usability, as evidenced by the manual derefs you needed to add

Right. Not strictly the result of the use of GAT - any type with only the deref bound would produce the same behavior.

  • this doesn't appear to improve performance at all

I hadn't measured, but I'll take your word for it.

Given the above, I don't see the benefit of making this change. Can you give your motivation for this? This is the second time you have wanted to change these methods in some way.

In this instance the motivation was that I found some code that had inadvertently taken a dependency on the concrete Reader type e.g. by using EndianSlice::to_{slice,string}. This was also the reason I (perhaps overly eagerly) removed the Result from to_slice, so now that I've restored that the benefit seems not as great as before.

philipc commented 1 year ago

I found some code that had inadvertently taken a dependency on the concrete Reader type e.g. by using EndianSlice::to_{slice,string}.

Why was that a problem? If you know you have an EndianSlice then there is nothing wrong with using it.

tamird commented 1 year ago

We were changing the implementation and the resulting compilation failures were confusing to at least one of us.

philipc commented 1 year ago

So the fundamental problem you are trying to solve is that Reader and EndianSlice have methods with the same name but different signatures? I agree that's a bit unfortunate, but I disagree that breaking changes and additional complexity are an appropriate solution.

tamird commented 1 year ago

Any solution would be necessarily breaking, wouldn't it?

While we're on the subject, to_string is especially unfortunate because the compiler wants to suggest ToString::to_string.

On Thu, Oct 26, 2023, 20:21 Philip Craig @.***> wrote:

So the fundamental problem you are trying to solve is that Reader and EndianSlice have methods with the same name but different signatures? I agree that's a bit unfortunate, but I disagree that breaking changes and additional complexity are an appropriate solution.

— Reply to this email directly, view it on GitHub https://github.com/gimli-rs/gimli/pull/682#issuecomment-1782107923, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALWYPHOJ5V7XAOCM7AHZHTYBL47BAVCNFSM6AAAAAA6PYXROOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBSGEYDOOJSGM . You are receiving this because you authored the thread.Message ID: @.***>

philipc commented 1 year ago

Any solution would be necessarily breaking, wouldn't it?

Probably, and so I don't think this is a problem that warrants fixing.

While we're on the subject, to_string is especially unfortunate because the compiler wants to suggest ToString::to_string.

to_string is the correct name for the operation that these methods are performing, and ToString doesn't appear anywhere here.

tamird commented 1 year ago

to_string is the correct name for the operation that these methods are performing, and ToString doesn't appear anywhere here.

ToString is in the std prelude, so the compiler always tries to suggest it for to_string with unfulfilled bounds.

philipc commented 1 year ago

Closing since I can't come up with an acceptable solution for this, and I consider this to be a minor problem that doesn't prevent you from writing correct code.