Fix translation of signed array indices

[travisstaloch]

• closes translate-c doesn't cast signed array index #4075

[daurnimator]

You only need the cast if the index is signed or has more bits than usize

[travisstaloch]

I thought there might need to be some checks like this.

Not sure how to do this right now. I'm guessing the result of ZigClangArraySubscriptExpr_getIdx(stmt) would contain the type info that would need checked. Right?

Here is the line in question:
79edfb9#diff-15590d874e44509c8710e7f4cddb840cR2413

[andrewrk]

Thanks!

Now that #4053 is merged, would you mind adding a "Run Translated C" test case?

file: test/run_translated_c.zig
cmd: ./zig build test-run-translated-c

This kind of test is better at catching whether translated-c code will actually turn into viable zig code.

[travisstaloch]

@andrewrk Should the tests be moved to test/run_translated_c.zig and removed from test/translate_c.zig or copied and changed?

[andrewrk]

For your pull request, I think it would be nice to copy&change, because it's also nice to see the input/output as a test case.

[travisstaloch]

I've managed to get this working with the checks for signedness and width.
However, my solution maybe hacky and wouldn't support casting long long indexes. I found qualTypeIntBitWidth but it doesn't include .LongLong and .ULongLong. Should these and other c builtin integer types be added here?

    switch (ZigClangType_getTypeClass(ty)) {
        .Builtin => {
            const builtin_ty = @ptrCast(*const ZigClangBuiltinType, ty);

            switch (ZigClangBuiltinType_getKind(builtin_ty)) {
                .Char_U,
                .UChar,
                .Char_S,
                .SChar,
                => return 8,
                .UInt128,
                .Int128,
// Should we add more types here such as:
//                .ULongLong,
//                .LongLong,
                => return 128,
                else => return 0,
            }

            unreachable;
        },

https://github.com/ziglang/zig/blob/master/src-self-hosted/translate_c.zig#L3043

[travisstaloch]

Would I be able to somehow get access to the QualType's TypeInfo? There seems to be public .Width property we could use: https://clang.llvm.org/doxygen/structclang_1_1TypeInfo.html. That is if we had access to QualType::getTypeInfo().

Or am I looking at the wrong docs here?

[travisstaloch]

"You only need the cast if the index is signed or has more bits than usize"
I have no problems checking for signedness.
@daurnimator @LemonBoy do you guys know how to properly check the width of a type from translate_c.zig? qualTypeIntBitWidth() could work but is likely intentionally incomplete as I noted two comments above.

[andrewrk]

Consider that the width of an integer type in bytes depends on the target, and when translating from C to Zig, generally you are translating target-agnostic code to target-agnostic code. So if you have to find out how many bytes an integer is exactly, it's a bit of a code smell.

There might be a related function that I already made for a different purpose, which selects a type based on another type, but overrides the signedness. Would that help?

[travisstaloch]

I think that would help. I'm working with a ZigClangQualType and trying to deduce the width from that.

[andrewrk]

zig/src-self-hosted/translate_c.zig

Lines 3097 to 3105 in 8c640b3

	/// Produces a Zig AST node by translating a Clang QualType, respecting the width, but modifying the signed-ness.
	/// Asserts the type is an integer.
	fn transQualTypeIntWidthOf(c: *Context, ty: ZigClangQualType, is_signed: bool) TypeError!*ast.Node {
	return transTypeIntWidthOf(c, qualTypeCanon(ty), is_signed);
	}
	/// Produces a Zig AST node by translating a Clang Type, respecting the width, but modifying the signed-ness.
	/// Asserts the type is an integer.
	fn transTypeIntWidthOf(c: *Context, ty: *const ZigClangType, is_signed: bool) TypeError!*ast.Node {

[travisstaloch]

Thanks for pointing out transQualTypeIntWidthOf.

So rather than checking the width, I'm now planning to create a narrow128BitIntType function which just maps .LongLong, .ULongLong, .Int128, .UInt128 => c_uint and leaves other types unchanged.

[andrewrk]

Mind rebasing while you're at it? This will make sure your new code doesn't regress the newly added test cases.

[LemonBoy]

Thousand dollar question, what about array[-1] ? That's legal C and definitely not the same as array[@bitCast(usize, @intCast(isize, -1))].

[travisstaloch]

I used to c_uint in this PR. Should these be usize instead?

Also I used @intCast rather than @bitCast.

[andrewrk]

array[-1]

(@as([*]E, &array) - 1).*

[LemonBoy]

(@as([*]E, &array) - 1).*

That's my point, should this PR generate that if the index is not unsigned?

Should these be usize instead?

When you're dealing with pointers using a usize is recommended.

Also, if you want to keep the PR tidy don't merge master into it but use git rebase instead.

[travisstaloch]

Apologies for the messy commits. I did rebase but ended up with conflicts and must have taken a wrong turn. Ended up needing git rebase --skip after resolving... 👎

using a usize is recommended

I'll update to usize then.

[travisstaloch]

I will clean up that nasty merge commit. Maybe it will be easier to leave as is for now and make a fresh PR if accepted.