Add additional binary operators to translate-c-2

[kavika13]

This pull request has a basis for implementing if/while/for (the code for handing boolean coercion).

Please code review. This was a bit trickier than some of my previous work, and I might be doing it incorrectly. I also fixed (I think) a thing I missed in my previous commits (rvalue vs lvalue mixup).

There's some tricky bits in the boolean expressions that still aren't handled correctly I believe, but I think they may not have been handled correctly in translate-c either.

E.g. coercing a typedef value to boolean. The old code I think would just spit out the typedef name instead of the typedef identifiers. The new code handles this a bit better, but still doesn't actually verify that it can just replace it with (val != 0) before doing that replacement, so it's possible it could create incorrect code sometimes.

I am noticing that there isn't really test cases for all translate-c code paths. Having someone write up more extensive and exhaustive translate-c test cases might be useful? They might be able to do examine the C grammar/AST somehow and create a bunch of samples. I am betting a lot of those tests would fail, too, but I imagine finding out how much coverage we have is a good goal anyway.

[Vexu]

Use ZigClangTypedefNameDecl_getUnderlyingType here, if then handle it if it is a builtin type.

[Vexu]

These should be an error.

[Vexu]

These should also be an error.

[Vexu]

Use Node.cast.

[kavika13]

Not sure I'm a fan of the fact that it can return null, might be better if it just does a compile time failure?

Or is that not possible in all scenarios? If so, maybe adding a separate tryCast would be a better option?

[Vexu]

You already checked that it is an InfixOp, it can't fail.

[kavika13]

If that's the case I might as well just do the cast, right?

Does the compile time check buy me anything? I guess it would give me a NULL segfault at runtime instead of reading random bits of the stack, in the odd chance I made a mistake or a refactoring somehow caused it to be wrong. But why does the cast bother making it a runtime failure, and not just compile error?

BTW I know I have to convert this to a parent from field cast at the very least.

[andrewrk]

@ptrCast is generally unsafe, and a blunt instrument that should be avoided when a more precise tool is available.

Indeed, your usage of @ptrCast here is incorrect, and is invoking undefined behavior. Zig does not guarantee order of struct fields; the base field of InfixOp is not guaranteed to be at offset 0. You'll notice that Node.cast uses @fieldParentPtr, not @ptrCast. The only reason your code runs in debug builds is because (1) you got (un)lucky and (2) #168 isn't implemented yet.

Node.cast is a "try cast", that's why it returns an optional pointer. res.node.cast(ast.Node.InfixOp).? does the cast, and asserts that the cast succeeded, returning a non-optional pointer.

Does the compile time check buy me anything?

There is no compile time check, it's doing a runtime check to make sure the type is correct. When you already did such a check (the switch) then the correct thing to do is assert that the check will succeed (by using .?).

I guess it would give me a NULL segfault at runtime instead of reading random bits of the stack, in the odd chance I made a mistake or a refactoring somehow caused it to be wrong.

Not a segfault, but an explicit panic message saying attempt to unwrap null.

But why does the cast bother making it a runtime failure, and not just compile error?

*Node is a runtime-known pointer, with a runtime-known id field. Such a compile error is impossible.

[andrewrk]

You might benefit with a structure more like this:

if (res.node.cast(ast.Node.InfixOp)) |infix_op| {
    // ...
} else if (res.node.cast(ast.Node.PrefixOp)) |prefix_op| {
    // ...
} else if (res.node.id == .BoolLiteral) {
    return res.node;
}

[kavika13]

When I read base.id == comptime typeToId(T), in my rush to get to sleep, I thought the whole expression was comptime. Indeed it's a runtime check because (Node*).id can't be known at compile time.

The code this comment is on compiled successfully when it was TransResult.node (for some reason). As you said, I got unlucky! However it fortunately doesn't compile now that TransResult was removed in a refactor.

I was going to solve this by swapping the code to use @fieldParentPtr(res, "base", ast.Node.InfixOp). Is there a reason the if(res.node.cast(ast.Node.Infix)) |infix_op| structure would be preferable?

[andrewrk]

@fieldParentPtr would be fine. My suggestion was based on semantic convenience. switch with @fieldParentPtr and my if-else chain there would do the same thing semantically, it's just a question of what is more maintainable.

[Vexu]

Due to how zig fmt works this op_token must be added before any of the rhs tokens. See

zig/lib/std/zig/render.zig

Line 1633 in 2c7a2ae

const rparen = tree.nextToken(if_node.condition.lastToken());

[Vexu]

lparen must also be added before any of the lhs tokens.

[Vexu]

fn transCreateNodeInfixOp(
    rp: RestorePoint,
    scope: *Scope,
    lparen: ?ast.TokenIndex,
    lhs_node: *ast.Node,
    op: ast.Node.InfixOp.Op,
    op_tok_id: ast.TokenIndex,
    rhs_node: *ast.Node,
) !*ast.Node {
    const node = try rp.c.a().create(ast.Node.InfixOp);
    node.* = .{
        .op_token = op_tok_id,
        .lhs = lhs_node,
        .op = op,
        .rhs = rhs_node,
    };
    if (lparen == null) return &node.base;
    const rparen = try appendToken(rp.c, .RParen, ")");
    const grouped_expr = try rp.c.a().create(ast.Node.GroupedExpression);
    grouped_expr.* = .{
        .lparen = lparen.?,
        .expr = &node.base,
        .rparen = rparen,
    };
    return &grouped_expr.base;
}

It's more verbose at call sites but it gives correct behavior.

[kavika13]

FYI, if you do end up taking on this task, I think passing in hard-coded parens here is not the right solution. I think it should just parse paren wrapped expression and translate them over as-is, if possible.

I believe I had this working earlier before restarting my work, and I don't think it was that hard to do.

If so, this transCreateNodeInfixOp function becomes quite a bit less verbose, so the duplication between it and other binary ops isn't a big deal at that point.

[andrewrk]

Thanks for the PR @kavika13! @Vexu if it's OK with you, then you're the official reviewer of this, and I'm happy to merge it when you feel it is ready.

Having someone write up more extensive and exhaustive translate-c test cases might be useful?

Yes this would be most welcome indeed.

[kavika13]

Thanks for the feedback @Vexu! I will look into the fixes you suggested a bit later in the week.