Skip to content

Added all missing double-pointer types in all source files - #122

Merged
suve merged 1 commit into
PascalGameDevelopment:masterfrom
flowCRANE:pointer-types
Apr 18, 2023
Merged

suve merged 1 commit into
PascalGameDevelopment:masterfrom
flowCRANE:pointer-types

Conversation

@flowCRANE

Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

First off, thanks a lot for this update which improves the units by a great margin.

Some ideas though:

  • the merge-commit #4dc9bad could have been prevented if the latest development state of the units would have been used
  • for review, tracking possible issues, and reverting undesired changes it makes life a lot easier if the changes are split over several commit instead of one large commit (I guess you used some algorithm for the updates - but even then it would be preferable to have the commits file by file or something)
  • I'm undecided about the double pointer types in ctypes.inc (e. g. ppcbool) because there is no counter-part in ctypes.pas (still approved form my side for the moment)

@flowCRANE

Copy link
Copy Markdown
Author

When it comes to GitHub, and especially contributing to open source, I have two left hands — I don't really understand how to click it correctly, so it's a big success anyway that the repository is still working. :)

@suve

suve commented Apr 4, 2023

Copy link
Copy Markdown
Collaborator

Ehh. Personally I'm really not a fan of this, but let's not repeat the discussion from #105 all over again.

There are some places where the indentation doesn't match, but if @Free-Pascal-meets-SDL-Website doesn't mind, I can merge this and fix misaligned places.

@flowCRANE

flowCRANE commented Apr 5, 2023 •

Copy link
Copy Markdown
Author

Indentations match everywhere, according to the types they are pointers to. I've checked this in both the IDE and GitHub.

Ehh. Personally I'm really not a fan of this, but let's not repeat the discussion from #105 all over again.

Even if these types are not used by headers, they will definitely be used by header users. The longer I developed my game code, the more double pointers I missed. Why would I declare them myself in the game code, when these types are strictly headers-related and should be provided by them? Well, that's why it's definitely better to include them in the headers and let everyone use what they need. At least these types will be properly declared and will always match the base header types.

If more types are added to the headers in the future, I suggest declaring a single and a double pointer for each of them. This way the headers will be fully usable and well organized, which is very important.

@Free-Pascal-meets-SDL-Website

Copy link
Copy Markdown
Collaborator

There are some places where the indentation doesn't match, but if @Free-Pascal-meets-SDL-Website doesn't mind, I can merge this and fix misaligned places.

Damn, missed these, saw them just on second inspection! Great job! I guess @furious-programming should have the chance to fix this as this is his/her PR.

Indentations match everywhere, according to the types they are pointers to. I've checked this in both the IDE and GitHub.

There are some mismatching indentations, see e. g. TSDL_SpinLock. Could you fix them, please.

If more types are added to the headers in the future, I suggest declaring a single and a double pointer for each of them. This way the headers will be fully usable and well organized, which is very important.

Please add as new fifth point (right before the point that links to the Tranlsation Cheat Sheet) in section Code style guidelines: For convenience we encourage to add single and double pointers for any SDL type. (See #105)

@flowCRANE

flowCRANE commented Apr 10, 2023 •

Copy link
Copy Markdown
Author

There are some mismatching indentations, see e. g. TSDL_SpinLock. Could you fix them, please.

I didn't even touch the TSDL_SpinLock declaration, so if the indentation is incorrect, it was incorrect all the time.

Edit: the indentation is incorrect, because one of the contributors uses Tabs instead of Spaces and the indentation is mixed. This is not visible in the Lazarus, since the IDE replace tabulators to spaces, but on GitHub this is visible. I'm using Spaces only (two for every indentation level), the changes made by me (concerning this pull request) was made in Lazarus (not in GitHub editor), so it's not my fault.

@Free-Pascal-meets-SDL-Website

Copy link
Copy Markdown
Collaborator

Thanks, from my side this is ready for merge now :-). Please just merge, if you agree @suve.

@suve

suve commented Apr 18, 2023

Copy link
Copy Markdown
Collaborator

I took the liberty to rebase the changes on top of latest master and fix any indentation issues I've found. If you say it looks okay now, then I'm going to merge it.

@suve
suve merged commit 4fe2132 into PascalGameDevelopment:master Apr 18, 2023
@flowCRANE
flowCRANE deleted the pointer-types branch April 18, 2023 20:00
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.

3 participants