-
Notifications
You must be signed in to change notification settings - Fork 271
cleanup AST builder functionality #1350
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
base: master
Are you sure you want to change the base?
Conversation
- remove unused function & impl - `impl<'a> Make<Unsafety> for &'a str` - `impl<'a> Make<Constness> for &'a str` - `impl Make<TokenStream> for Vec<String>` - `impl Make<PathArguments> for ParenthesizedGenericArguments` - `fn parenthesized_args` - refactor mk().vis() usage so we can remove `impl<'a> Make<Visibility> for &'a str` - refactor mk().set_mutbl() usage so we can remove `impl<'a> Make<Mutability> for &'a str` - refactor mk().unary_expr() usage so we can remove `impl<'a> Make<UnOp> for &'a str` - unify `impl Make<TokenStream> for Vec<&str>`, `impl Make<TokenStream> for Vec<u64>`, and `impl Make<TokenStream> for Vec<Meta>` functionality to use comma_separated function
@@ -667,12 +667,13 @@ impl StructureState { | |||
/// * Negating something of the form `!<expr>` produces `<expr>` | |||
/// | |||
fn not(bool_expr: &Expr) -> Box<Expr> { | |||
use syn::UnOp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this can be hoisted to the top-level, that'd be preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can create a new module, lets call it transform
, which contain all helper function that transform a rust AST to another rust AST. so we can put helper function like this not()
and unparen()
in there. However i prefer doing that in another PR rather than this
@@ -336,82 +290,29 @@ impl Make<TokenStream> for Vec<TokenTree> { | |||
} | |||
} | |||
|
|||
impl Make<TokenStream> for Vec<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With an iterator-based fn comma_separated
, you can simplify this to:
impl Make<TokenStream> for Vec<&str> {
fn make(self, _mk: &Builder) -> TokenStream {
comma_separated(self.iter().map(|&s| Ident::new(s, Span::call_site())))
}
}
impl Make<TokenStream> for Vec<u64> {
fn make(self, _mk: &Builder) -> TokenStream {
comma_separated(self.iter().map(|&s| Literal::u64_unsuffixed(s)))
}
}
impl Make<TokenStream> for Vec<Meta> {
fn make(self, _mk: &Builder) -> TokenStream {
comma_separated(self.iter())
}
}
fn comma_separated<T, F>(items: Vec<T>, f: F) -> TokenStream | ||
where | ||
F: Fn(&mut TokenStream, T), | ||
{ | ||
let mut tokens = TokenStream::new(); | ||
let mut first = true; | ||
for item in items { | ||
if !first { | ||
TokenTree::Punct(Punct::new(',', Spacing::Alone)).to_tokens(&mut tokens); | ||
} else { | ||
first = false; | ||
} | ||
f(&mut tokens, item); | ||
} | ||
tokens | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn comma_separated<T, F>(items: Vec<T>, f: F) -> TokenStream | |
where | |
F: Fn(&mut TokenStream, T), | |
{ | |
let mut tokens = TokenStream::new(); | |
let mut first = true; | |
for item in items { | |
if !first { | |
TokenTree::Punct(Punct::new(',', Spacing::Alone)).to_tokens(&mut tokens); | |
} else { | |
first = false; | |
} | |
f(&mut tokens, item); | |
} | |
tokens | |
} | |
fn comma_separated<I, T>(items: I) -> TokenStream | |
where | |
I: Iterator<Item = T>, | |
T: ToTokens + Clone, | |
{ | |
let items = items.map(|items| items.to_token_stream()); | |
let comma = TokenTree::Punct(Punct::new(',', Spacing::Alone)).into_token_stream(); | |
intersperse(items, comma).collect() | |
} |
If you add itertools
to c2rust-ast-builder
(which we already use for c2rust-transpile
), this could be a lot simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it is ok to add new dependencies crate, i will add it tomorrow. cannot wait for rust-lang/rust#79524 to become stable😃
"deref" | "*" => UnOp::Deref(Default::default()), | ||
"not" | "!" => UnOp::Not(Default::default()), | ||
"neg" | "-" => UnOp::Neg(Default::default()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure getting rid of all of these is helpful. "!"
is pretty easy to read and understand (although things like "not"
and the other non-standard spellings for things seem bad). It has the downside of not being as easily searchable since it's not tied to a symbol like UnOp::Not
, but it's also easier to read and write in a bunch of ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is more readable for all unary_expr() usage in transpile to just use mk().deref_expr()
, mk().neg_expr()
, and mk().not_expr()
. I can create follow up PR for the refactor
impl<'a> Make<Unsafety> for &'a str
impl<'a> Make<Constness> for &'a str
impl Make<TokenStream> for Vec<String>
impl Make<PathArguments> for ParenthesizedGenericArguments
fn parenthesized_args
mk().vis()
usage so we can removeimpl<'a> Make<Visibility> for &'a str
mk().set_mutbl()
usage so we can removeimpl<'a> Make<Mutability> for &'a str
mk().unary_expr()
usage so we can removeimpl<'a> Make<UnOp> for &'a str
impl Make<TokenStream> for Vec<&str>
,impl Make<TokenStream> for Vec<u64>
, andimpl Make<TokenStream> for Vec<Meta>
functionality to usefn comma_separated