search

remobjects / pascalscript

pascalscript
Other
447 stars 178 forks source link

Bug fixes & changes in InvokeCall.inc #225

Open Vizit0r opened 4 years ago

Vizit0r commented 4 years ago

1) added type "Variant" to params types list 2) added Interface to result types list 3) Changed flag IsStatic for x64 for "String" type, so Strings will be passed and returned to\from Delphi methods correctly in x64. 4) added notice about using types-synonyms

Issue numbers:

219

218

217

Vizit0r commented 4 years ago

@martijnlaan

carlokok commented 3 years ago

@Vizit0r Can you check the conflict? I'll try to merge it next week.

Vizit0r commented 3 years ago

@Vizit0r Can you check the conflict? I'll try to merge it next week.

@carlokok

Negative, atm i'm at work with only smartphone on hand. Will back home after 3-4 week only.

The best will be to wait, if possible. I will check it all again once will be near home pc, and report to you.

pult commented 3 years ago

Some ideas for "openarray,record,set,fpc,xe3" https://github.com/pult/pascalscript/blob/pult/Source/InvokeCall.inc

Vizit0r commented 3 years ago

@carlokok i remember 1 thing, whick cause changes all over the PS files. in Delphi in x64 POSIX "LongInt" type will have 8 bytes size. All other systems - 4 bytes size. In FPC this type have constant size 4b anywhere. same for Integer - in FPC in x64 it has 8b size, in Delphi anywhere - 4b. My decision was to make define

{$IFDEF FPC} LongInt4b = LongInt; {$ELSE} LongInt4b = Integer; {$ENDIF}

but in this case ALL "LongInt" in ALL PS files must be changed to LongInt4b. Looks not so good. Without this changes - all is dying in POSIX x64.

All other changes still checking. Also, will look @pult InvokeCall.inc

Vizit0r commented 3 years ago

@pult if possible, contact me plz in Discord, my id is @Vizit0r#4772 , I have a couple of question about FPC compilation, types etc. Much better will be to produce compatible code together, than make query of fix commits.

Vizit0r commented 3 years ago

Discord not work for me :( Telegram: ....

Joined.

Vizit0r commented 3 years ago

@pult i cant write you, thats not a chat, but channel.

Vizit0r commented 3 years ago

@carlokok i have finished all. As described in last commit, big problems with FPC there. Need someone, who have good skills in FPC, to finish its part in InvokeCall.inc. In Delphi all works fine. Tests for checking included in folder "Invoke Tests" All tested in Delphi, all systems, including Android 64. Also read remark in commit about calling script methods from Delphi in non-Win systems.

martijnlaan commented 3 years ago

This is pull request shouldn't be merged as-is in my opinion: the commits contain lots of undocumented and therefore unexplained changes.

I still believe #205 and #207 should be reverted to bring ROPS back to a usable state.

Vizit0r commented 3 years ago

okey guys, revert changes, as you want. If nobody interested in multiplatform - no problem. Sit more on Win only, with existed documented and explained (really, no) code. For me and two hundreds of my users it works fine on Linux&Windows&Macos, thats was my goal. I try to publish my code for community, to make a pleasure for other developers, but if the problem is "undocumented code" - okey, forgot it.

Tests - its also nothing, code is undocumented.

Normally, who wants to see the explanation - read a commits comments. Have no idea, why you dont want to go this way.

Good bye, and happy new year.

martijnlaan commented 3 years ago

Just giving my opinion, it's up to Carlo what happens.

It's a fact though that your commits contain changes which don't match the commit descriptions like https://github.com/remobjects/pascalscript/pull/225/commits/8ef297c93e7678db5a79efe9a679f3ab56ba70d4 which has an additional debugger related change and https://github.com/remobjects/pascalscript/pull/225/commits/3b2cabe5f06adbc3d7870d0c2369d8cca450ea43 which has an additional change that looks like a memory leak fix. These are just 2 examples. These changes might be good and intentional but that can't be seen from your descriptions.

I've pointed this out in earlier commits by you which also contained unexplained changes.

One of the commits has a change to uPSRuntime so large that Github can't even display it.

This isn't how you're supposed to make pull requests. Here's a good explainer if you're interested: https://www.jamescroft.co.uk/a-guide-to-making-a-good-pull-request/ The biggest problem of a pull request like this one that it's way too big and doesn't focus on a specific area, making it impossible to properly review.

Vizit0r commented 3 years ago

i've not comment EVERY small change, if its not critical or important. F.e. in https://github.com/remobjects/pascalscript/commit/8ef297c93e7678db5a79efe9a679f3ab56ba70d4 adding one more condition for checking in "LineInfo" - thats for preventing error, which my user have, very rare error, but its exists. Thats much more explanation, what its done for, than real code. Others same.

I have a lot of changes, which i dont include in my commits, because i'm not sure it can be useful for someone else.

About the rules of commiting - i've never read it, works only with repos alone or in small commands, there were no any problems with my commitment style.

Anyway, @carlokok should decide. If question only in commits styling, i can spent a lot of time and divide them again, but size of some of them cant be decreased.

zedxxx commented 3 years ago

Resolves #241

Vizit0r commented 3 years ago

Additionally, I beleive Data^[l] and Items[l] point to the same thing so this change doesn't do anything except making the 2 places inconsistent.

Thats jut to to access field in same manner, as all over the PS code.

@Vizit0r The old code where it uses Data is still present in TPSExec.Destroy. Shouldn't it be changed there too?

i dont make an aim to change it anywhere, what i see - i change to common manner.