-
Notifications
You must be signed in to change notification settings - Fork 20
Added all missing double-pointer types in all source files #122
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
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.
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)
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. :)
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.
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.
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)
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.
Thanks, from my side this is ready for merge now :-). Please just merge, if you agree @suve.
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.
No description provided.