Skip to content
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

Update to Lazarus 1.8RC5. #410

Closed
wants to merge 2 commits into from
Closed

Update to Lazarus 1.8RC5. #410

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 17, 2017

The Lazarus 1.8RC5 release has very few breaking changes. 1.8 nears release, and it seems wise to focus development efforts on this milestone.

There are a number of FIXME items in the code stemming from upstream changes to the LCL.

Copy link
Collaborator

@JohnPeel JohnPeel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks fine.

See my line comments, and we'll look into updating the LCL wrappers as well.

@@ -101,7 +101,8 @@ TFunctionListFrame = class(TFrame)
implementation

uses
SimbaUnit, Graphics, stringutil, simpleanalyzer, v_ideCodeParser, lclintf, dynlibs;
SimbaUnit, Graphics, stringutil, simpleanalyzer, v_ideCodeParser, lclintf,
dynlibs, LazFileUtils;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LazUTF8 needs to go here, correct?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The modifications compile as they are. I only added the new unit when there were errors and didn't end up adding it in some files I expected to add it to. The changes were fairly complicated and I am not exactly sure why some functions were moved.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we used the UTF8 functions in this file before it will need LazUTF8 now.

Just because it compiles, doesn't mean it is correct.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really any unit that uses non-hardcoded file access should have it.

Especially any units used to give scripts access to file handling.

Copy link
Author

@ghost ghost Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default types of keywords changed, and the type system should be ensuring that the proper functions are being used. It is not always necessary to use special UTF8 functions and some of the Lazarus 1.8 updates made changes in this vein.

Later units mask earlier ones. Some functions in FileUtils will mask the functions in LazUTF8 and LazFileUtils. I encountered this in a few places, I think this is one. Some of the imports might be redundant. Separate PRs to refactor the imports might be good to ask for, as well as other tidying-up.

This is also a response to your comment below. Some units have ordering requirements, but I didn't find any between those two.

@@ -581,6 +586,8 @@ implementation
uses
lclintf,
syncobjs, // for the critical sections / mutexes
LazUTF8,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, LazUTF8 has to be after LazFileUtils.

@MerlijnWajer
Copy link
Collaborator

Merged into fpc-3.0

@MerlijnWajer
Copy link
Collaborator

Also - thanks!

@ollydev ollydev mentioned this pull request May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants