transpile: const macros: strip implicit casts from macro expansion sites#1311
transpile: const macros: strip implicit casts from macro expansion sites#1311fw-immunant merged 9 commits intomasterfrom
Conversation
|
After cherry-picking: onto this branch and running pub const silk_int16_MIN: std::ffi::c_short = 0x8000 as std::ffi::c_int
as std::ffi::c_short;
#[no_mangle]
pub unsafe extern "C" fn test_silk_int16_MIN() -> std::ffi::c_int {
let mut _null: std::ffi::c_char = (*::core::mem::transmute::<
&[u8; 1],
&[std::ffi::c_char; 1],
>(b"\0"))[(silk_int16_MIN as std::ffi::c_int + 0x8000 as std::ffi::c_int) as usize];
return silk_int16_MIN as std::ffi::c_int;
}That looks sane to me (the const is -32768). Am I missing something? |
|
Hmm, that is correct. I just wasn't seeing that in the snapshot. If you rebase onto that branch and check in the updated snapshot, it should be all good. It just wasn't working for me for some reason. |
|
@fw-immunant, can I rebase this? |
1255ca4 to
d0c85d0
Compare
kkysen
left a comment
There was a problem hiding this comment.
@fw-immunant, I'm not sure this is the right approach. It makes the translation much worse (see the snapshot diff).
604c58a to
cdb7bd0
Compare
|
Here's my reasoning around this change:
We call this in If we avoid calling I'll look into whether there's some way to determine which casts are caused by the expansion context as opposed to being part of the macro itself, as the former as the only ones we should strip off to generate the translation for the macro body. |
kkysen
left a comment
There was a problem hiding this comment.
I'll look into whether there's some way to determine which casts are caused by the expansion context as opposed to being part of the macro itself, as the former as the only ones we should strip off to generate the translation for the macro body.
If we're able to do that, that sounds like it would be good. Pulling in all the contextual casts not written in the macro itself, though, like this does, just seems like it results in a much worse translation, although I understand the reasoning for it.
81551c0 to
ecddc95
Compare
1815f3e to
cc1c9cc
Compare
ea88c30 to
033afab
Compare
103cc62 to
642de3c
Compare
10f58bd to
35e058d
Compare
| if ctx.is_const { | ||
| return Err(format_translation_err!( | ||
| None, | ||
| "cannot check nullity of pointer in `const` context", | ||
| )); | ||
| } | ||
| let is_null = mk().method_call_expr(e, "is_null", vec![]); | ||
| if negated { | ||
| mk().unary_expr(UnOp::Not(Default::default()), is_null) | ||
| } else { | ||
| is_null | ||
| } |
There was a problem hiding this comment.
How come we can't use fn match_bool here?
There was a problem hiding this comment.
I think we plausibly could; I wasn't looking for refactorings here, and the code is slightly different (here we only handle pointer types).
c2rust-transpile/tests/snapshots/snapshots__transpile-linux@macros.c.snap
Outdated
Show resolved
Hide resolved
| use CExprKind::*; | ||
| match expr { | ||
| ImplicitCast(_, subexpr, _, _, _) => { |
There was a problem hiding this comment.
If it's only one match arm and not a ton of them, a glob import probably isn't worth it.
| use CExprKind::*; | |
| match expr { | |
| ImplicitCast(_, subexpr, _, _, _) => { | |
| match expr { | |
| CExprKind::ImplicitCast(_, subexpr, _, _, _) => { |
There was a problem hiding this comment.
Could also be an if let. This likely won't change, right, as it's pretty specific to implicit casts?
a00692a to
ce3c96d
Compare
| // name of the const macro only occurs if we process the expr_id with a direct call | ||
| // to `convert_expr`. |
There was a problem hiding this comment.
When/where does this happen? That might've been part of what was tripping me up in adding override_tys to const macros.
There was a problem hiding this comment.
At the beginning of convert_expr:
pub fn convert_expr(
&self,
mut ctx: ExprContext,
expr_id: CExprId,
override_ty: Option<CQualTypeId>,
) -> TranslationResult<WithStmts<Box<Expr>>> {
let Located {
loc: src_loc,
kind: expr_kind,
} = &self.ast_context[expr_id];
trace!(
"Converting expr {:?}: {:?}",
expr_id,
self.ast_context[expr_id]
);
if let Some(converted) = self.convert_const_macro_expansion(ctx, expr_id, override_ty)? {
return Ok(converted);
}There was a problem hiding this comment.
Oh, sorry, I meant the opposite. When do we not go through fn convert_expr to process an expr_id?
There was a problem hiding this comment.
Whenever we look extra levels down in the expression tree and short-circuit instead of recursing in convert_expr; there isn't a concise answer here, but it mostly happens toward leaves of the tree.
ce3c96d to
843d67a
Compare
843d67a to
6656cd2
Compare
430cc73 to
25fdd7a
Compare
resolving the type ignores all casts, but some casts are nontrivial computations that we cannot elide, e.g. truncating larger types to smaller ones as far as I'm aware we can simply not resolve the expression type here; we'll emit more casts in these cases, but some of those may be load-bearing
…ting two levels deep in casts
c2e07f6 to
816b7b3
Compare
kkysen
left a comment
There was a problem hiding this comment.
I think it LGTM now. I'm not sure why SIG_IGN isn't being translated as a const now (I saw it was in one commit), though.
Still needs tests.