Skip to content

Conversation

@suve
Copy link
Collaborator

@suve suve commented Apr 18, 2023

This patch fixes the definitions for PSDL_Mutex, PSDL_Sem and PSDL_Cond, adds a proper definition for SDL_MUTEX_MAXWAIT, and restores original SDL doc-comments for the mutex functions.

suve added 3 commits April 18, 2023 20:58
Previously, these types were aliases to the un-typed pointer type,
which meant that any pointer value could be assigned to them.
They are now declared as distinct pointer types.
@Free-Pascal-meets-SDL-Website
Copy link
Collaborator

  • Thanks, looks good so far. Does this patch clean up to 2.0.14 or a later version?
  • The "WIP" tag should be removed from the version tag in sdl2.pas.

Best regards
Matthias

@suve
Copy link
Collaborator Author

suve commented Apr 19, 2023

With this patch, sdlmutex.inc matches SDL_mutex.h as of SDL 2.26.3 - though to be fair, the API has not changed since SDL 2.0.0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please have a look at the change proposals.

Best regards
Matthias

{ The SDL mutex structure, defined in SDL_sysmutex.c }
PPSDL_Mutex = ^PSDL_Mutex;
PSDL_Mutex = Pointer; //todo!
PSDL_Mutex = Type Pointer;

Choose a reason for hiding this comment

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

This should better be

type
  PPSDL_Mutex = ^PSDL_Mutex;
  PSDL_Mutex = ^TSDL_Mutex;
  TSDL_Mutex = record end;

right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess we can use the record notation for now and later have a separate PR where we get rid of all the faux TSDL_Something types and replace PSDL_Something with Type Pointer.

Choose a reason for hiding this comment

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

I see,
you never stated your opinion on this topic (exact translation of opaque structs), which was discussed in issue #63.

In short: From my point of view, which admittetedly is heavily influenced by a translator-view, the translation of

/* The SDL mutex structure, defined in SDL_sysmutex.c */
struct SDL_mutex;
typedef struct SDL_mutex SDL_mutex;

is better reflected by

TSDL_mutex = record end;

rather than by

PSDL_mutex = type Pointer;

I understand, that formally it would not be necessary to declare the TSDL_mutex type if it is not used in the header code though, but wouldn'd call this necessarily faux'ed. I consider this kind of a dilemma.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't think much of it when the question was first asked, basically following a "if it works, don't break it" principle - but over time, I came to favour the type Pointer solution, since by avoiding having a TSDL_Something declaration in the first place, it's made quite clear the user is not expected to ever use the non-pointer type.

Choose a reason for hiding this comment

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

I see your point, it would make use of the units safer in some regard. Well, then let's make both ways possible for now. An update of the code style guidelines has to be done then. Let me know, if I should do it.

We could think about a goal for a future version of the units to only favor the pointer-translation for consistency.

Best regards

@Free-Pascal-meets-SDL-Website
Copy link
Collaborator

Thanks for the clean up!

I will merge this and update the version tag and the cheatsheet hint separately and directly to the master branch. This is quicker than pushing to this PR as these files are not included, yet.

@Free-Pascal-meets-SDL-Website Free-Pascal-meets-SDL-Website merged commit 15930c7 into PascalGameDevelopment:master Jun 9, 2023
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.

2 participants