Change function args from `String` to `Into<String>`

[icefoxen]

Description

Doesn't help quite as much as one might hope because there's still lots of structures that you build literally, and those all contain String and kind of have to. If you want me to make constructor functions for those I will, but that's an API decision I happily leave up to you.

Fixes #14

Changes proposed in this pull request

As in the description

ToDo

• [x] Proposed feature/fix is sufficiently tested
• [x] Proposed feature/fix is sufficiently documented
• [x] The "Unreleased" section in the changelog has been updated, if applicable

[garritfra]

Thanks for your PR!

Doesn't help quite as much as one might hope because there's still lots of structures that you build literally, and those all contain String and kind of have to.

I assume you mean structures like Value::Temporary("foo".into()), right? I'm not sure how or if this should be changed, but I'm absolutely open for suggestions.

In #12 we went the route of adding lifetimes to types which would make it possible to embed a str ref as a type, though I'm not sure if this is a route worth taking. What do you think?

One more minor thing: I think it would be nice to have a simple test case to assure that both "foo".into() and "foo" are valid parameters for the functions changed by this PR. Do you mind adding this? 🙂

[icefoxen]

I assume you mean structures like Value::Temporary("foo".into()), right? I'm not sure how or if this should be changed, but I'm absolutely open for suggestions.

Yeah, those. I don't know about should, the only way I can think of that it could be changed is making methods for Value::new_temporary("foo"). All that really does is change one boilerplate for another. I'd probably do it, to save boilerplate for the user versus the lib writer, but it's really a matter of taste.

In https://github.com/garritfra/qbe-rs/pull/12 we went the route of adding lifetimes to types...

I don't know, I tend to avoid making structs with lifetimes in them, even if it means more cloning or an Rc or something. I'll take a closer look at that and see how it feels.

I think it would be nice to have a simple test case to assure that both "foo".into() and "foo" are valid parameters for the functions changed by this PR.

Good call, I'll add it!

[garritfra]

All that really does is change one boilerplate for another. I'd probably do it, to save boilerplate for the user versus the lib writer, but it's really a matter of taste.

Hmm, I'm still undecided about this. I'd usually say that adding unnecessary boilerplate is just a burden to maintain, but since we went with constructors for other structures as well, I'd probably add it to keep a more or less consistent API. You have my go!

[icefoxen]

Added some basic unit tests. But on the subject of actually allowing "foo".into() to be accepted by the constructor functions... well, it results in this error:

error[E0283]: type annotations needed
   --> src/tests.rs:87:17
    |
87  |     let func3 = Function::new(Linkage::public(), "main".into(), Vec::new(), None);
    |                 ^^^^^^^^^^^^^                    ------------- this method call resolves to `T`
    |                 |
    |                 cannot infer type for type parameter `impl Into<String>` declared on the associated function `new`
    |
    = note: cannot satisfy `_: Into<String>`
note: required by a bound in `Function::<'a>::new`

Basically, it's expecting "main".into() to turn the &str into some type that is impl Into<String>, but since &str is already impl Into<String> it doesn't really know what to do with it.

[garritfra]

Basically, it's expecting "main".into() to turn the &str into some type that is impl Into<String>, but since &str is already impl Into<String> it doesn't really know what to do with it.

hmm, tricky...

I just had a look around, and it seems like taking a generic AsRef<str> is the way to go here. What do you think?

[icefoxen]

That might work better, I'll try it out. Might involve an extra clone() here and there but that is far from a concern for me.

[garritfra]

Hi! Sorry for keeping you waiting for so long. I just did a deep dive into variants of the string type, and impl Into<String> does seem to be the proper type in our case. This answer explains it very well.

Thanks for your work on this!