hrp: add constructor from an arbitrary T: Display

[apoelstra]

Fixes #195

[tcharding]

Doesn't this macro call have to allocate a String because data is not know until runtime?

My understanding is that write! calls format_args! which creates an Arguments type which has an as_str method but only if there are no runtime args.

In other words isn't this the same performance as:

    pub fn parse_display<T: core::fmt::Display>(data: T) -> Result<Self, Error> {
        let s = data.to_string();
        Self::parse(&s)
    }

[apoelstra]

This code works without any allocator. It's not allocating a string. It would also be insane for the fmt::Write API to work this way, where every single call to write_str involved a gratuitous allocation.

[apoelstra]

Or indeed, for every call to write! anywhere to involve gratuitous allocations.

If you think this could be true we'd need to change our fmt::Display implementations throughout every one of our crates to avoid this macro.

[apoelstra]

It took me a little while to trace through the code but the core of write! is the write function which iterates over the "pieces" of an Arguments and does not allocate.

[tcharding]

Cool, TIL. Thanks for the link (I was unconvinced before it, you saved me trawling through the source :)

[tcharding]

There are a few problems here, I uncovered them by writing a test to check the code comment (which is incorrect, we return the last error), I think we want break instead of continue.

Also we need to iterate over chars and check they are ascii instead of using bytes - we do this in parse above.

[tcharding]

Instead of commenting each line I just wrote some tests that I think should pass

    #[test]
    fn parse_non_ascii() {
        assert_eq!(Hrp::parse("❤").unwrap_err(), Error::NonAsciiChar('❤'));
    }

    #[test]
    fn parse_display_non_ascii() {
        assert_eq!(Hrp::parse("❤").unwrap_err(), Error::NonAsciiChar('❤'));
    }

    #[test]
    fn parse_display_returns_first_error() {
        assert_eq!(Hrp::parse_display("❤ ").unwrap_err(), Error::NonAsciiChar('❤'));
    }

    // This test shows that the error does not contain heart.
    #[test]
    fn parse_display_iterates_chars() {
        // FIXME: Once test above passes the error char will need changing.
        assert_eq!(Hrp::parse_display(" ❤").unwrap_err(), Error::NonAsciiChar('❤'));
    }

[apoelstra]

Thanks! I will take these tests and make them pass.

[apoelstra]

Should be good now.

[tcharding]

ACK 2fdc674

[tcharding]

nit: This comment is stale now I believe.

[tcharding]

I think we could just check the length at the start and early return.

[apoelstra]

I don't see why this is stale. The line is called every time the write_str function is called, for the reason that the comment describes.

We could do a length check at the start and early return but I don't see what that would accomplish other than increasing the cyclotomic complexity of the function.

[tcharding]

ooo, I see. Don't mind me.

[apoelstra]

I think we may want to backport this to 0.11, since when we release 0.12, rust-bitcoin 0.32 won't be able to use it (thanks to rust-bitcoin/rust-bitcoin#3043).

Same with #197, so let's wait until that's done as well.

[clarkmoody]

Should this call parse_display here?

[apoelstra]

Oops, yes! Fixed.

[tcharding]

A bug in the unit tests, what are the odds ;)

[tcharding]

Oh actually, if this is based on the unit test code I posted above I intentionally included one that parsed using Hrp::parse.

[apoelstra]

Yes, you intentionally included one, but you unintentionally included another one :).

[tcharding]

Sweet, cheers. Its hard to get good help :)

[clarkmoody]

ACK 11d6cb4