-
-
Notifications
You must be signed in to change notification settings - Fork 324
changes to fix Lazarus and FreePascal compilation issues. #413
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
Conversation
current git release wasn't compiling under Lazarus/fpc, so here is the fix.
now it builds, i guess you did not test it with fpc for a long time, and it did not build without the fixes in this commit.
i hope i did everything right. feel free to edit my code.
I see the issue. I just suggested some simplifications.
I see the issue. I just suggested some simplifications.
Yes, please suggest. Or you can apply the pull request and then apply your own improvements.
I see the issue. I just suggested some simplifications.
maybe I did not notice, where did you suggest some changes?
i guess you were doing code review and forgot to press 'submit review' button.
Source/PythonEngine.pas
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NewSize := Utf8ToUnicode(PWideChar(Result), Cardinal(Size + 1), Buffer, Cardinal(Size));
Would work both in Delphi and Fpc.
PUnicodeChar is an alias to PWideChar;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true!
Source/WrapDelphiClasses.pas
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DelphiObject := TResourceStreamClass(DelphiObjectClass).Create(LHandle, String(LResName), PChar(String(LResType)))
Should work in both fpc and Delphi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Source/WrapDelphiClasses.pas
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Source/WrapDelphi.pas
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i: Integer
should be in the {$ELSE} part of the the IFDEF below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I addressed all the comments.
Thank you for those. You can take a look at another commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant in the ELSE part of EXTENDED_RTTI! You don't need IFDEF FPC. And use Integer instead of longint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, now I see.
Thank you for comments and your patience.
Now I did it, but PR contains 3 commits.
Do you want me to figure the way to change it for 1 commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, do you like the current state of it?
would you merge? or should i change something else?
i guess you were doing code review and forgot to press 'submit review' button.
Indeed.
hey, sorry for pinging you again.
but would you like to merge it? or did you solve it yourself already?
Sorry for the delay. Very busy right now. I will deal with PR in a couple of weeks.
No description provided.