New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest using Cow on mixture of &'static str + Strings #320

Open
llogiq opened this Issue Sep 8, 2015 · 8 comments

Comments

Projects
None yet
2 participants
@llogiq
Collaborator

llogiq commented Sep 8, 2015

I see a lot of owned strings in many places where a Cow<'static, str> will do. The latter will likely improve performance.

How do we detect this? It's a value of type String which are only initialized with other Strings from elsewhere or &'static str (via .to_owned()/.to_string()). Also we should probably check if the value is part of the public interface (as we did recently with wrong_pub_self_convention).

I think this should be possible with the ExprUseVisitor – so I finally have a reason to check it out. 😄

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 14, 2015

Collaborator

Revisiting this, it is not clear to me if the ExprUseVisitor really can do the things we need.

First, we need to look at the whole crate. We start from a set of every bound value, struct/tuple field or enum variant content that is of type String. We then check all initializations and modifications of our String and remove it from the set if it contains a .to_owned(), .to_string(), .from() or .into() for a &str with any but static lifetime. Then we remove all Strings from the set that are part from the public API (e.g. returned from a function, etc.). We also need to keep track of all uses of our String – if at least one of them takes an &str, we should report the declaration, all initializations/assignments and all uses of the string, and for each suggest a code replacement that uses Cow<'static, str> instead (if there is any difference).

So if we want to use the ExprUseVisitor, I think our delegate needs to have a Map of Paths (or something different that we can use to distinguish those things) to a struct with the exprs of all initializations, assignments and uses of the String (we can use parenting to find out how it's used) plus a flag that tells us to stop...or perhaps an Option<(Vec<&'tcx Expr>, Vec<&'tcx Expr>, Vec<&tcx Expr>)>).

Collaborator

llogiq commented Sep 14, 2015

Revisiting this, it is not clear to me if the ExprUseVisitor really can do the things we need.

First, we need to look at the whole crate. We start from a set of every bound value, struct/tuple field or enum variant content that is of type String. We then check all initializations and modifications of our String and remove it from the set if it contains a .to_owned(), .to_string(), .from() or .into() for a &str with any but static lifetime. Then we remove all Strings from the set that are part from the public API (e.g. returned from a function, etc.). We also need to keep track of all uses of our String – if at least one of them takes an &str, we should report the declaration, all initializations/assignments and all uses of the string, and for each suggest a code replacement that uses Cow<'static, str> instead (if there is any difference).

So if we want to use the ExprUseVisitor, I think our delegate needs to have a Map of Paths (or something different that we can use to distinguish those things) to a struct with the exprs of all initializations, assignments and uses of the String (we can use parenting to find out how it's used) plus a flag that tells us to stop...or perhaps an Option<(Vec<&'tcx Expr>, Vec<&'tcx Expr>, Vec<&tcx Expr>)>).

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 14, 2015

Collaborator

I also need to implement Visitor somewhere, because I need a point when the whole crate is checked to report all instances of Cowifiable Strings. This allows me to None out the nodes that are public.

Collaborator

llogiq commented Sep 14, 2015

I also need to implement Visitor somewhere, because I need a point when the whole crate is checked to report all instances of Cowifiable Strings. This allows me to None out the nodes that are public.

@llogiq llogiq added E-hard and removed E-medium labels Sep 14, 2015

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 14, 2015

Collaborator

I think this is medium to get at all, but hard to get right.

@Manishearth am I on the right track? Or is there an easier path?

Collaborator

llogiq commented Sep 14, 2015

I think this is medium to get at all, but hard to get right.

@Manishearth am I on the right track? Or is there an easier path?

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 15, 2015

Collaborator

I started a cow branch to get my code online. I think I have the Visitor part down (there's the open question of generics, but I'm just ignoring them for now).

Now I need to implement the Delegate for the ExprUseVisitor and grab an InferCtxt to fill my map. Then I somehow need to use that map for reporting.

Collaborator

llogiq commented Sep 15, 2015

I started a cow branch to get my code online. I think I have the Visitor part down (there's the open question of generics, but I'm just ignoring them for now).

Now I need to implement the Delegate for the ExprUseVisitor and grab an InferCtxt to fill my map. Then I somehow need to use that map for reporting.

@llogiq llogiq self-assigned this Sep 15, 2015

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 15, 2015

Collaborator

@birkenfeld I seem to have problems getting the lifetimes right. Halp 😅

Collaborator

llogiq commented Sep 15, 2015

@birkenfeld I seem to have problems getting the lifetimes right. Halp 😅

@birkenfeld

This comment has been minimized.

Show comment
Hide comment
@birkenfeld

birkenfeld Sep 17, 2015

Collaborator

@llogiq how can I help?

Collaborator

birkenfeld commented Sep 17, 2015

@llogiq how can I help?

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 17, 2015

Collaborator

I posed a question on stackoverflow which luckily got the correct answer. The reason for the problem was that I had a wrong lifetime for the Delegate.

Collaborator

llogiq commented Sep 17, 2015

I posed a question on stackoverflow which luckily got the correct answer. The reason for the problem was that I had a wrong lifetime for the Delegate.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 18, 2015

Collaborator

This is a lot more complicated than I'd thought. I have to find local variables (similar to shadow), struct fields/enum variant args (I don't have anything similar yet), etc. to bind the "use" to the correct identity. I need a check if a String comes from a borrowed str. I also need to care about things returned from the function (because the function may return String).

Collaborator

llogiq commented Sep 18, 2015

This is a lot more complicated than I'd thought. I have to find local variables (similar to shadow), struct fields/enum variant args (I don't have anything similar yet), etc. to bind the "use" to the correct identity. I need a check if a String comes from a borrowed str. I also need to care about things returned from the function (because the function may return String).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment