nielsAD / lape

Scripting engine with Pascal-like syntax for FPC and Delphi
118 stars 28 forks source link

Math operations? #188

Closed Alekcvp closed 1 year ago

Alekcvp commented 1 year ago

What versions of Delphi are considered compatible?

When compiling in version 11, there are a lot of warnings and as a result everything works like this:

var
  s: Int64 = -1148327974329;
  us: UInt64 = 8473289;
begin
  WriteLn(s);
  WriteLn('-------------- = ', s div us);
  WriteLn('       ', us);
end;
Compiling Time: 31ms.
-1148327974329
-------------- = 2177046354182
       8473289
Running Time: 0ms.
slackydev commented 1 year ago

What you are describing is a result of bad coding practices.

Delphi will treat division between unsigned and signed datatype as if both values was unsigned, C and C++ will do the same. FPC however does not do it this way, it treats both as signed.

The base idea is that in programming, you want to avoid mixing various datatypes as this forces the compiler to do automatic/implicit casting which may impact your results, and cause unexpected behavior. And how the automatic casting is done can vary between compilers, if they even allow this. Go language for example will not allow you to do what you are doing right now. You would be forced to cast it yourself, explicitly, which you should at the very least do in your script if you have to mix datatypes like that.

var
  s: Int64 = -10000;
  us: UInt64 = 100;
begin
  WriteLn(s);
  WriteLn('-------------- = ', s div Int64(us));
  WriteLn(us);
end; 
Alekcvp commented 1 year ago

Delphi will treat division between unsigned and signed datatype as if both values was unsigned Yes, I know. The problem is that the lape code itself contains tons of code where unsigned numbers are mixed with signed ones. As an example:

procedure lpeInt64_IDIV_UInt64(const Dest, Left, Right: Pointer); {$IFDEF Lape_CDECL}cdecl;{$ENDIF} 
begin 
  PInt64(Dest)^ := **PInt64**(Left)^ div **PUInt64**(Right)^; 
end;

As a result, a lot of warnings occur during compilation, and I gave the result above. In addition, Delphi warns in such cases, but the lape just gives the wrong result.

P.S: https://onlinegdb.com/JIuvnJS7D

ollydev commented 1 year ago

Delphi 11: It does warn, but gives the same result as lape in delphi (and c++).

Project1.dpr(15): W1073 Combining signed type and unsigned 64-bit type - treated as an unsigned type

image

slackydev commented 1 year ago

Yes, this is done deliberately. We want the compiler to make those decisions, rather than lape. This allows code that's written in lape to better resemble code that is written in whatever compiler you choose to use for lape.

Alekcvp commented 1 year ago

And at the same time, it makes debugging critically difficult. Because lack of warnings + a typo in one character can force the whole code to be inspected...

And if this is done deliberately, then it might be worth turning off warnings when compiling lape?

slackydev commented 1 year ago

And at the same time, it makes debugging critically difficult. Because lack of warnings + a typo in one character can force the whole code to be inspected...

Honestly, I dont follow. Debugging your script? Using a debugger on your project in Delphi? And what do you mean by a "typo in one character"?

The issue at hand, which was raised, which is regarding math operations between signed and unsigned: No matter how we choose to do it, either it's cast to unsigned, or it's cast to signed, in both case we lose something. A UInt64 cast to Int64 will invalidate half it's range (the biggest numbers half), while Int64 cast to UInt64 invalidate the lover range (negatives). Mixing like this is bad code practices, CPU can't do binary ops on (UInt64, Int64) combo, there will be loss, and it can cause rollover no matter which way we cast.

And if this is done deliberately, then it might be worth turning off warnings when compiling lape?

This is up to you. We will not disable this in the project file (at this moment at least).

Alekcvp commented 1 year ago

Honestly, I dont follow. Debugging your script? Using a debugger on your project in Delphi? And what do you mean by a "typo in one character"?

I meant that if you misspelled somewhere by writing Uint instead of Int, or vice versa, then without warning you can search for this place for a long time.

This is up to you. We will not disable this in the project file (at this moment at least). Not in the entire project, but only in those places where it is planned.

slackydev commented 1 year ago

I meant that if you misspelled somewhere by writing Uint instead of Int, or vice versa, then without warning you can search for this place for a long time.

Well, the outcome would be the same if we enforced UInt64 downcast to Int64, rather than Int64 upcast to UInt64: -- Possibility of unexpected behavior.

Sure I agree that adding compiler hints for implicit typecasts of binary operations can be beneficial. But it's at best low priority.

Not in the entire project, but only in those places where it is planned.

Those compiler hints mean nothing to you when you compile, you have my permission to ignore them. If anything they can be a clear hint to lape users about how we inherit implicit binary operator casting from the compiler, so they should not really be hidden by default.

ollydev commented 1 year ago

Closing this as it's not a "bug". Compiler hints on integer overflow/casting should be added eventually. I did do a commit to fix Lape compiling and have all tests pass on Delphi (11.3, Windows). Hope it helps, just note 99.9% of Lape usage is from FPC/Lazarus. 32423d0738cc763ad5f58d3f58718a2d7b6b35f0

Alekcvp commented 1 year ago

Thank you.

P.S: Is this double conversion exactly required? PlpString(Result)^ := UTF8ToString(UTF8Encode(PUnicodeString(Params^[0])^));

ollydev commented 1 year ago

Thank you.

P.S: Is this double conversion exactly required? PlpString(Result)^ := UTF8ToString(UTF8Encode(PUnicodeString(Params^[0])^));

UTF8Encode returns RawByteString and Delphi produces a small compiler warning like "possible data loss" when converting. For such a explict conversion method it's probably better to ensure it's all covered.

The entire method is probably pointless though as the default string type is UnicodeString.

Alekcvp commented 1 year ago

I mean: https://docwiki.embarcadero.com/Libraries/Alexandria/en/System.UTF8Encode Call UTF8Encode to convert a Unicode string to UTF-8. https://docwiki.embarcadero.com/Libraries/Sydney/en/System.UTF8ToString Converts a UTF-8 encoded string to a string.

So, PlpString(Result)^ := UTF8ToString(UTF8Encode(PUnicodeString(Params^[0])^)); is equal to PlpString(Result)^ := PUnicodeString(Params^[0])^;, isn't it?

Method must return AnsiString type (utf8string) to be useful.

ollydev commented 1 year ago

Correct, will fix soon.