Add support for expressions like (0 << SHIFT) | BOOL

[jaseemabid]

The PR adds support for parenthesized expressions, identifiers and bit wise or operators. Fixes #412.

See individual commits for details.

When handling identifiers, I've resorted to a naive impl like this

if segments.len() == 1 {                
    Ok(Literal::Expr(format!("{}", segments.last().unwrap().ident)))

Let me know if this is not OK, I'm not sure if ir::Path is really necessary in this case.

[emilio]

Thanks for the patch! This looks like the right direction, just some concerns, and we should fix the interaction with the renaming options, so that it doesn't break builds.

[emilio]

This should be a different kind of Literal, so that it can work correctly with export.prefix and such configuration options. rename_for_config should be called on it.

Please add a test for this to the prefix tests.

[jaseemabid]

This should be a different kind of Literal

@emilio Just to be clear, did you mean I should add a variant to enum ir::constant::Literal to handle names instead of reusing Expr? That would indeed be better. What do you think we should call it? Name/Reference/Segment/Path ?

Makes sense about renaming for config, will do that and add a test 👍

[emilio]

Yeah. Literal::Path would make sense to me, wdyt?

[jaseemabid]

Literal::Path(String) sounds good!

[emilio]

This is pretty footgunny C code. We should probably parenthesize all binary ops when emmitting to a macro. That would also avoid the need for Literal::Paren, I think, which would maybe make the patch a bit simpler.

[jaseemabid]

@emilio

This is pretty footgunny C code. We should probably parenthesize all binary ops when emmitting to a macro.

I agree, this can go wrong in so many ways - happy to add a parenthesis around the whole expression if there is a binary op present. Should be able to peek into the structure inside the write function do this.

That would also avoid the need for Literal::Paren, I think, which would maybe make the patch a bit simpler.

I agree, adding that variant did make me cringe a bit. I prefer to not have it there.

Do you think I should just recursively call the value inside and avoid the wrapping all together? Something like this?

- syn::Expr::Paren(syn::ExprParen { ref expr, .. }) => Ok(Literal::Paren {
-     inner: Box::new(Self::load(expr)?),
- }),

+ syn::Expr::Paren(syn::ExprParen { ref expr, .. }) => Self::load(expr),

[emilio]

Yeah, thtat should work, but let me know if it doesn't.

[jaseemabid]

I was slightly sceptical but this seems to have worked fine, working on a few tests now to make sure the config and renaming work correctly.

[jaseemabid]

@emilio Thanks for the initial review, v2 looks much better.

The prefix tests seems to be renaming things alright, there is no Paren anymore and operator precedence around () seems to be working as expected.

[emilio]

Yeah, this looks neat, and much simpler, thanks!

Looks good to me :)

[jaseemabid]

Thanks @emilio 🙏