cranelift / PR #1231 Print return values of run tests if ... · git-cranelift

We shouldn't panic on parse errors, we should return a fallible ParseResult like other parsing functions do.

Additionally, it seems a little strange that we have this parsing code in the cranelift-filetests crate, when AFAICT all the other parsing code is in the cranelift-reader crate. Unless there is a good reason not to, we should match existing conventions and keep the parsing code inside cranelift-reader.

GitHub (Jan 06 2020 at 20:59):

fitzgen created PR Review Comment:

It isn't clear to me what the resolution is here, if any.

GitHub (Jan 06 2020 at 20:59):

fitzgen created PR Review Comment:

Was this still going to happen?

GitHub (Jan 06 2020 at 20:59):

fitzgen created PR Review Comment:

I believe that moving that parsing code into this cranelift-reader crate would let us avoid making all these methods public as well.

GitHub (Jan 06 2020 at 20:59):

fitzgen created PR Review Comment:

Instead of making an ad-hoc try_into method, we should follow convention and implement the TryFrom trait, which gives us a try_into via the blanket impl TryInto<T> for U where U: TryFrom<T>. For details, see: https://doc.rust-lang.org/stable/std/convert/trait.TryInto.html and https://doc.rust-lang.org/stable/std/convert/trait.TryFrom.html

If I'm reading this code right, it would be something like this:
struct $name(ConstantData);

impl TryFrom<$name> for $type {
    // ...
}
But this also makes me wonder why we don't implement TryFrom<ConstantData> for $type directly instead of creating all these temporary ConstantData$Type types. Is that something that you explored at all? I think it would be the best course to take, unless it doesn't work for some reason or I am overlooking something else.

GitHub (Apr 04 2020 at 20:56):

krk closed without merge PR #1231.

Last updated: Nov 22 2024 at 17:03 UTC