vishapoberon / compiler

vishap oberon compiler
http://oberon.vishap.am
GNU General Public License v3.0
186 stars 25 forks source link

FOR i := 0 TO Args.argc - 1 DO -> incompatible assignment #77

Closed jkleiser closed 4 years ago

jkleiser commented 4 years ago

This looks like a bug to me. When I try TO Args.argc - 1 DO in the FOR statement, I get this:

  12:   FOR i := 0 TO Args.argc - 1 DO
                                  ^
    pos   290  err 113  incompatible assignment

but this compiles:

FOR i := 0 TO 10 - 1 DO

Do I need to cast Args.argc to some other kind of integer?

btreut commented 4 years ago

just a wild guess: I think you declared i as INTEGER and you are importing argc from V4/Args. If so, then argc is declared as LONGINT. So either declare i as LONGINT too or use SHORT(Args.argc).

jkleiser commented 4 years ago

Thanks! SHORT(Args.argc) seems to be the best solution. I first tried VAR i: LONGINT, but then I got conflict with Args.Get(i, arg).

jkleiser commented 4 years ago

Isn't it a bit strange that argc-: INT32 while PROCEDURE Get(n: INT16; ...?

DEFINITION Args;

  VAR
    argc-: INT32;
    argv-: INT64;

  PROCEDURE Get(n: INT16; VAR val: ARRAY OF CHAR);
  PROCEDURE GetEnv(var: ARRAY OF CHAR; VAR val: ARRAY OF CHAR);
  PROCEDURE GetInt(n: INT16; VAR val: INT32);
  PROCEDURE Pos(s: ARRAY OF CHAR): INT16;
  PROCEDURE getEnv(var: ARRAY OF CHAR; VAR val: ARRAY OF CHAR): BOOLEAN;

END Args.
btreut commented 4 years ago

absolutely, but that might be due to legacy backward compatibility

dcwbrown commented 4 years ago

Looking back I believe I broke this, and that argc should indeed be defined as INTEGER.

Therefore I propose to correct argc-: LONGINT to argc-: INTEGER in Args.mod.

There is a risk and I'm looking for advice on how much to worry. The issue I see is that where someone has coded SHORT(Args.argc), on 16 bit INTEGER system, their program would previously have supported up to 32767 arguments (!), but now it will only support up to 127 arguments.

I'm mostly inclined to think this is acceptable - what do you think?

jkleiser commented 4 years ago

To me, changing that one from LONGINT to INTEGER looks like a safe thing to do. But I'm quite new in this voc field.

norayr commented 4 years ago

I vote for your change, David, I think there is not much code to break now, and we have luxury to follow better design decisions.

dcwbrown commented 4 years ago

Thanks all, I've changed Args.argc from LONGINT to INTEGER.

jkleiser commented 4 years ago

Great. After a git pull, make full and make install, I can now compile without the SHORT on Args.argc. However, when I do showdef Args, I get the same old

DEFINITION Args;

  VAR
    argc-: INT32;
    argv-: INT64;

  PROCEDURE Get(n: INT16; VAR val: ARRAY OF CHAR);
  PROCEDURE GetEnv(var: ARRAY OF CHAR; VAR val: ARRAY OF CHAR);
  PROCEDURE GetInt(n: INT16; VAR val: INT32);
  PROCEDURE Pos(s: ARRAY OF CHAR): INT16;
  PROCEDURE getEnv(var: ARRAY OF CHAR; VAR val: ARRAY OF CHAR): BOOLEAN;

END Args.

How do I refresh this?

dcwbrown commented 4 years ago

All I can think of is that you are running showdef in a directory that has an old copy of Args.sym in it. Is that possible?

(Showdef could do with a bit of work - it could include a comment at the start identifying the exact symbol file it is dumping, and it could substitute INTEGER for INT16 and similarly for all the other model dependent types.)

-- Dave.

jkleiser commented 4 years ago

What I had to do to refresh it, was copy the new Args.sym from voc/install/2/sym/ to /opt/voc/2/sym/ . When I did the make install, after I pulled and did the make full, I got the message "make: Nothing to be done for 'install/'." ;-)

dcwbrown commented 4 years ago

Re

make: Nothing to be done for 'install/'.

It looks like you may have typed

sudo make install/

instead of

sudo make install

?

jkleiser commented 4 years ago

That is possible, or maybe I forgot sudo, or both.

norayr commented 4 years ago

i would suggest just to remove the destination directory (/opt/voc ?) and do make install again.

jkleiser commented 4 years ago

I pulled on another Mac today, and now I took care, doing the sudo make install correctly. Now showdef Args showed the new INT16 type for argc. Sorry for the noise. ;-)