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

Fields initialized as array literals in a class' defaults generate invalid C code #912

Closed
alexnask opened this issue Jul 15, 2015 · 7 comments

Comments

@alexnask
Copy link
Collaborator

Here are the problematic C sources (come from sdk/os/native/TerminalWin32.ooc):

void os_native_TerminalWin32__TerminalWin32___defaults___impl(os_native_TerminalWin32__TerminalWin32* this) {
    os_Terminal__TerminalHandler___defaults___impl((os_Terminal__TerminalHandler*) this);
    this->bg = 0;
    this->fg = 8;
    this->colors = (lang_Memory__memcpy(__os_native_TerminalWin32_arrLit1.data, __os_native_TerminalWin32_ptrLit2, 9 * ((lang_types__Class*)lang_Numbers__Int_class())->size), __os_native_TerminalWin32_arrLit1);
    #line 25 "/Users/amos/Dev/ooc/rock/sdk/os/native/TerminalWin32.ooc"
    _lang_array__Array __os_native_TerminalWin32_arrLit1 = _lang_array__Array_new(lang_Numbers__Int, 9);
    #line 25 "/Users/amos/Dev/ooc/rock/sdk/os/native/TerminalWin32.ooc"
    lang_Numbers__Int* __os_native_TerminalWin32_ptrLit2 = (lang_Numbers__Int[]) { 0, 12, 10, 14, 9, 13, 11, 7, 31 };
}

The following code is produced by the following ooc code:

version(windows) {
// Some winapi binding happening here...
    TerminalWin32: class extends TerminalHandler {
        bg := Color black
        fg := Color white

        init: func

        /* Color codes */
        // Here is the issue (this should also be static or even better, an enum)
        colors := [
            0 , // black
            12, // red
            10, // green
            14, // yellow
            9 , // blue
            13, // magenta
            11, // cyan
            7 , // grey
            31  // white
        ]
        // Implements the Terminal API from hereon...
    }
}

I'm also getting the "undefined reference to `GC_beginthreadex'" error when I correct the C code manually, I've gotten around that one before though, I'll figure it out.

Any idea why the array literal is defined after the actual variable?
Perhaps some issue with array members in version blocks?
I can't really find another testcase atm, I will come up with one if I manage to get rock to compile though.

@fasterthanlime
Copy link
Collaborator

Hmm did I upload a bad 0.9.11-prealpha1 bootstrap? Will have to check.

os/Terminal could definitely use a bit of a cleanup. As for GC_beginthreadex, I suppose it's a boehm gc lib clash again.. if it was picking up the one compiled from rock's Makefiles, it should have all the threading support we need. (Hmm.. or not, maybe it does need --enable-threads=win32 explicitly for windows).

The wrong order for literals is something I've seen in the middle of my cleanups, but I think it was before the 0.9.10 release. Might have been after and I'm mistaken, though.

@alexnask
Copy link
Collaborator Author

By manually switching around the C code and using the instructions from here, I managed to get c_rock, I do get 'C compiler failed on module os/native/TerminalWin32 from sdk, bailing out' on make self though, so I assume the C code generated is still invalid, will tkae a look now.

(By the way, c_rock -V output is rock 0.9.11-head codename sapporo, built on Sun Jul 12 15:53:51 2015)

@fasterthanlime
Copy link
Collaborator

Ah yes, actually the release was made here: 3f04249

Looking at the history for ArrayLiteral, it looks like it hasn't been touched after July 10? Would that mean that it is still broken?

Actually, that's completely plausible, since the travis tests don't run on Windows, the windows code doesn't even compile.

@horasal
Copy link
Contributor

horasal commented Jul 16, 2015

A simple test:

Foo: class{
    test := [1, 2, 3]
    init: func
}

describe("array literial should be correctly unwrapped in default",|| Foo new())

horasal added a commit to horasal/rock that referenced this issue Jul 16, 2015
@fasterthanlime
Copy link
Collaborator

@zhaihj if at all possible, I prefer tests to do some runtime checking as well. That test might very well compile but put garbage inside the array, and we wouldn't know (and it's happened before..)

@alexnask
Copy link
Collaborator Author

As a temporary workaround, I applied @zhaihj's patch and commented out the failing code in TerminalWin32.ooc, then made rock with c_rock (OOC=bin/c_rock make self)

And then, of course, uncommented the code and ran a regular make self

@alexnask alexnask changed the title Make rescue fails on windows (provided C sources have an error) Fields initialized as array literals in a class' defaults generate invalid C code Jul 16, 2015
@alexnask alexnask removed the Win32 label Jul 16, 2015
@fasterthanlime
Copy link
Collaborator

Closed by #915!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants