Add SDL3_net#50
Conversation
|
Finished and ready for review. |
Perfect. Indeed I was eying this "project" already. :D I'm gonna review soon. |
Free-Pascal-meets-SDL-Website
left a comment
Free-Pascal-meets-SDL-Website
left a comment
There was a problem hiding this comment.
Hi there,
just reviewed your contribution. Basically it is well done, as always.
I'd suggest some minor changes as hinted to by the comments:
- usually the type def. order is double pointer (first), pointer (second) and type (third); that is how it is throughout our project; while reviewing I wondered, if the other way round would be better in that the comment above the type def. would be bound to the type and not to the double pointer def. as you usually are interested in the original type rather than one of the pointer types (should we change that? What's your opinion?)
- although not needed, I'd suggest to close even one line statements in functions by semi-colon
Now, this is important (therefore request changes status instead of approve): Please update the README.MD:
- remove "(not published yet)" from the unit list (Installation part)
- add "adds SDL3_net unit" in the versions part
Best regards
Matthias
| * | ||
| * \since This enum is available since SDL_net 3.0.0. | ||
| *) | ||
| TNET_Status = type cint; |
There was a problem hiding this comment.
For consistency, we usually do it in this order:
doublepointer def
pointer def
type def (below)
There was a problem hiding this comment.
Yeah, as you noticed in the main comment, I did it like this so the doc-comment is attached to the t-def, not the pp-def.
There was a problem hiding this comment.
Okay, so ignore the def-order comments then. We should update the style guide in this regard and make it flexible (allow both ways) or prefer the "new" order with this reasoning. What would you say?
| * \sa NET_GetLocalAddresses | ||
| * \sa NET_CompareAddresses | ||
| *) | ||
| PNET_Address = Type Pointer; |
There was a problem hiding this comment.
Minor 1: "type" lower case
Minor 2: switch order of defs
| * \sa NET_WriteToStreamSocket | ||
| * \sa NET_ReadFromStreamSocket | ||
| *) | ||
| PNET_StreamSocket = type Pointer; |
There was a problem hiding this comment.
Switch order of defs
| * \sa NET_CreateServer | ||
| *) | ||
| PNET_Server = type Pointer; | ||
| PPNET_Server = ^PNET_Server; |
There was a problem hiding this comment.
Switch def order
| * \sa NET_GetStreamSocketPendingWrites | ||
| * \sa NET_WaitUntilStreamSocketDrained | ||
| *) | ||
| procedure NET_DestroyStreamSocket(sock: PNET_StreamSocket); cdecl; |
There was a problem hiding this comment.
Missing inline comment: "/* Destroy your sockets when finished with them. Does not block, handles shutdown internally. */"
Not sure why this comment is inlined. Should we add it?
| * \sa NET_SendDatagram | ||
| * \sa NET_ReceiveDatagram | ||
| *) | ||
| PNET_DatagramSocket = type Pointer; |
There was a problem hiding this comment.
Switch def order
| buf: pcuint8; (**< the payload of this datagram. *) | ||
| buflen: cint; (**< the number of bytes available at `buf`. *) | ||
| end; | ||
| PNET_Datagram = ^TNET_Datagram; |
There was a problem hiding this comment.
Switch def order
|
|
||
| function SDL_NET_VERSION(): Integer; | ||
| begin | ||
| Result := SDL_VERSIONNUM(SDL_NET_MAJOR_VERSION, SDL_NET_MINOR_VERSION, SDL_NET_MICRO_VERSION) |
There was a problem hiding this comment.
Close by semi colon
| begin | ||
| Result := (SDL_NET_MAJOR_VERSION >= major) and | ||
| ((SDL_NET_MAJOR_VERSION > major) or (SDL_NET_MINOR_VERSION >= minor)) and | ||
| ((SDL_NET_MAJOR_VERSION > major) or (SDL_NET_MINOR_VERSION > minor) or (SDL_NET_MICRO_VERSION >= micro)) |
There was a problem hiding this comment.
Close by semi colon
2 participants
Work in progress. Opening the PR early to avoid duplicating work.