c2lang / c2compiler

the c2 programming language
c2lang.org
Apache License 2.0
704 stars 49 forks source link

Analyser does not detect first case after label is declaration #78

Closed lerno closed 5 years ago

lerno commented 5 years ago

This code gladly compiles:

switch (foo) {
  case 1:
    i32 a = 1;
}

However, this is illegal code when compiled to C:

switch (argc) {
  case 1:
  int32_t a = 1;
}
lerno commented 5 years ago

Probably the best would actually to allow this and put a noop in front of the case when generating to C.

luciusmagn commented 5 years ago

Yea, usually, I just put a semicolon after the colon as an innocuous hack. Doing something similar would be very nice for the ergonomics of the language

Dne st 28. 11. 2018 0:10 uživatel Christoffer Lerno < notifications@github.com> napsal:

Probably the best would actually to allow this and put a noop in front of the case when generating to C.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/c2lang/c2compiler/issues/78#issuecomment-442253832, or mute the thread https://github.com/notifications/unsubscribe-auth/AIAzh30zjvcwIc-hGUsoW19r0CMTBKx1ks5uzcZagaJpZM4Y2hm5 .

bvdberg commented 5 years ago

I wanted to allow declarations inside a case statement. So the proper c-generation should be:

case 1: {
    i32 a = 1;
   break;
}

This does not look nice, but that's why C2 allows declarations inside. Ideally, c2c should only generate extra braces if needed. I'll look into this..

lerno commented 5 years ago

Note that adding {} does change behavior on fallthrough. And also affects the defer implementation.

lerno commented 5 years ago

I think the ; hack would be a nice thing. Then we mirror most of C’s behaviour and don’t introduce arbitrary scopes. On the other hand, maybe we want that? If switch has break by default (and I think I had some nice syntax for that) then scope could indeed be based on case. I personally feel that this might be useful.

(My suggestion on switch/case/break:

switch (foo) {
 case 1 |
 case 2:
   // above was fallthrough
   auto_break_after_this();
 case 3:
   foo();
   fallthru;
 case 4:
   bar();
}

)

lerno commented 5 years ago

fallthrough might be better as goto next or goto case 4

bvdberg commented 5 years ago

Fixed properly by putting case in own scope. No real overhead during analysis and generation. This also fixed an issue I discovered during fixing that this would work:

case 1:
   i32 a = 10;
   break;
case 2:
   // a was usable here

Let's put the other fallthrough/etc discussions on the forum first..

lerno commented 5 years ago

Note that the above is valid C, so you're making a change to how C works. To understand why this is valid, it's useful to think of the cases as labels and the switch as a goto.

lerno commented 5 years ago

Also, it has some major repercussions on how defer works. Consider this Defer code (not possible to solve using RAII in C++!) that worked up until your change:

switch (state) {
  case READ_WITH_LOCK:;
    LOCK *lock = acquire_lock();
    defer lock.release();
  case READ_WITHOUT_LOCK:
    read_from_resource();
    break;
  case ...
}

If you add the implicit scope for lines with declarations, it will be translated to this:

switch (state) {
  case READ_WITH_LOCK: {  
    LOCK *lock = acquire_lock();
    defer lock.release();
  }
  case READ_WITHOUT_LOCK:
    read_from_resource();
    break;
  case ...
}

In the first case we can trigger the defer on leaving the switch scope (which we want), but in the second case the declaration is not visible in the top switch scope, and consequently the defer cannot access it. Defer must therefore trigger before entering the READ_WITHOUT_LOCK case

lerno commented 5 years ago

I have opened a fallthrough discussion here: http://www.c2lang.org/forum/index.php?topic=147.0