Skip to content
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

Translate-c-2 the rest #3935

Merged
merged 38 commits into from Dec 22, 2019
Merged

Translate-c-2 the rest #3935

merged 38 commits into from Dec 22, 2019

Conversation

Vexu
Copy link
Member

@Vexu Vexu commented Dec 17, 2019

Statements

  • StmtExprClass
  • CompoundAssignOperatorClass
  • UnaryOperatorClass
  • CallExprClass
  • MemberExprClass
  • ArraySubscriptExprClass
  • UnaryExprOrTypeTraitExprClass
  • Fix issues with visitFnDecl

Casts

  • PointerToBoolean
  • IntegralToBoolean
  • PointerToIntegral
  • IntegralToPointer

Macros

@daurnimator daurnimator added the translate-c C to Zig source translation feature (@cImport) label Dec 18, 2019

// TODO https://github.com/ziglang/zig/issues/3756
// TODO https://github.com/ziglang/zig/issues/1802
const checked_name = if (isZigPrimitiveType(var_name)) try std.fmt.allocPrint(c.a(), "_{}", .{var_name}) else var_name;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct in thinking this introduces a workaround for #3756?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I don't like the how it makes translated names slightly less predictable, but at least it won't break on import.

@kavika13
Copy link
Contributor

kavika13 commented Dec 18, 2019

I noticed that the translate-c tests are failing on master for me (even without my changes), and I'm not sure the cause. I also can't separate those bugs from anything my changes have caused, so I am a little blocked.

I'm going to be mostly unavailable for the holidays pretty shortly, so if you want to hijack my pull request into your work then please go for it!

@Vexu
Copy link
Member Author

Vexu commented Dec 18, 2019

I noticed that the translate-c tests are failing on master for me (even without my changes), and I'm not sure the cause. I also can't separate those bugs from anything my changes have caused, so I am a little blocked.

Seems like an issue with transImplicitCastExpr.

I'm going to be mostly unavailable for the holidays pretty shortly, so if you want to hijack my pull request into your work then please go for it!

Will do.

@Vexu
Copy link
Member Author

Vexu commented Dec 18, 2019

Seems like an issue with transImplicitCastExpr.

I think I already fixed this.

}
const lhs_node = try transExpr(rp, scope, ZigClangBinaryOperator_getLHS(stmt), .used, .l_value);
switch (op) {
.PtrMemD, .PtrMemI, .Cmp => return revertAndWarn(
Copy link
Contributor

@kavika13 kavika13 Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think .* (PtrMemD) and ->* (PtrMemI) should be unreachable, because they're C++ things?
https://github.com/llvm-mirror/clang/blob/master/include/clang/AST/OperationKinds.def#L357

Also the spaceship <=> (Cmp) operator:
https://github.com/llvm-mirror/clang/blob/master/include/clang/AST/OperationKinds.def#L370

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely agree, thanks for looking them up.

lib/std/zig/ast.zig Outdated Show resolved Hide resolved
lib/std/zig/ast.zig Outdated Show resolved Hide resolved
@Vexu Vexu marked this pull request as ready for review December 19, 2019 22:10
@data-man
Copy link
Contributor

int main() {
  int len = 2;
  switch (len) {
    case 0:
      break;
    case 1 ... 3:
      break;
    case 4 ... 6:
      break;
    default:
      ;
  }
  return 0;
}

=>

pub export fn main() c_int {
    var len: c_int = 2;
    __switch: {
        __default: {
            __case_2: {
                __case_1: {
                    __case_0: {
                        switch (len) {
                            0 => break :__case_0,
                            1...1 => break :__case_1,
                            4...4 => break :__case_2,
                            else => break :__default,
                        }
                    }
                    break :__switch;
                }
                break :__switch;
            }
            break :__switch;
        }
        {}
    }
    return 0;
}

Of course, this is GCC's extension, and there is not much of a code like that.

@andrewrk
Copy link
Member

@Vexu after this is merged I would like to invite you to do the honors of deleting the C++ implementation from the repo. I think you deserve to have all those deleted lines under your name 😁

@Vexu
Copy link
Member Author

Vexu commented Dec 22, 2019

I went ahead and made all non-namespaced enums integers to match stage 1 behavior with anonymous enums. I also made the change for named enums since the current implementation of transEnumDecl doesn't know if it is actually anonymous or a typedef.

The benefit from this is that if you need an integer you can just use the identifier as you would in C and if you need an enum all you need to do is prefix the it with a period.

- make non-namespaced enums ints
- fix .used compound assignments not being grouped
- fix macro calls with casts producing invalid Zig
@data-man
Copy link
Contributor

//#include <stdint.h>

int a = 1;
int64_t b = 1;

ints.c:4:1: error: unknown type name 'int64_t'
int64_t b = 1;
^

If #include is uncommented then

pub const uint_least64_t = u64;
pub const int_fast64_t = i64;
pub const uint_fast64_t = u64;
pub const int_least32_t = i32;
pub const uint_least32_t = u32;
pub const int_fast32_t = i32;
pub const uint_fast32_t = u32;
pub const int_least16_t = i16;
pub const uint_least16_t = u16;
pub const int_fast16_t = i16;
pub const uint_fast16_t = u16;
pub const int_least8_t = i8;
pub const uint_least8_t = u8;
pub const int_fast8_t = i8;
pub const uint_fast8_t = u8;
pub const intmax_t = c_long;
pub const uintmax_t = c_ulong;
_pub export var a: c_int = 1;_
_pub export var b: i64 = @as(i64, 1);_
			^ Why export?
const __INTMAX_TYPE__ = @compileError("unable to translate macro");
const __UINTMAX_TYPE__ = @compileError("unable to translate macro");
const __PTRDIFF_TYPE__ = @compileError("unable to translate macro");
const __INTPTR_TYPE__ = @compileError("unable to translate macro");
const __SIZE_TYPE__ = @compileError("unable to translate macro");
const __WINT_TYPE__ = @compileError("unable to translate macro");`

...and many errors

Would be nice not to take integer definitions from clang.

@Vexu
Copy link
Member Author

Vexu commented Dec 22, 2019

_pub export var a: c_int = 1;_
_pub export var b: i64 = @as(i64, 1);_
			^ Why export?

Because they are not static, see #2780

Would be nice not to take integer definitions from clang.

Currently this behaves same as stage 1 which also requires you to include stdint.h

@andrewrk
Copy link
Member

Ready to merge?

@Vexu
Copy link
Member Author

Vexu commented Dec 22, 2019

Yes, works as good as stage1 on everything I've tried.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am delighted to merge this enormous contribution to Zig. Excellent work, @Vexu.

@andrewrk andrewrk merged commit 28dbdba into ziglang:master Dec 22, 2019
@Vexu Vexu deleted the translate-c-2 branch December 22, 2019 21:15
@andrewrk
Copy link
Member

@Vexu want to do the honors of hooking up @cImport to the self-hosted version and deleting the C++ implementation?

@Vexu
Copy link
Member Author

Vexu commented Dec 22, 2019

I've already done so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
translate-c C to Zig source translation feature (@cImport)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants