lclevy / ADFlib

A free, portable and open implementation of the Amiga filesystem
GNU General Public License v2.0
90 stars 30 forks source link

Improvements in the adf_file API #47

Closed t-w closed 1 year ago

t-w commented 1 year ago
  1. Changed way of defining the mode in adfFileOpen, the 2 modes replace as follows:

    • "r" -> ADF_FILE_MODE_READ
    • "w" -> ADF_FILE_MODE_READWRTIE
  2. Added a few useful functions (very simple - static inline):

    • for checking status (position, size)
      • checking EOF changed also to static inline for consistency
    • seeking (start, EOF)
t-w commented 1 year ago

Question here:

I was thinking about changing struct AdfFile ... to typedef struct AdfFile {...} AdfFile. This would simplify using the interface (struct everywhere adds unnecessary clutter, it is clear that this is a custom type).

But probably the same would have to be applied to other modules for consistency (either now, for 0.8.0, or later - for me both are fine).

What do you think?

lclevy commented 1 year ago

Seek from current is useful. And can be : A = current_pos + delta Seek (start +a)

⁣Télécharger BlueMail pour Android ​

Le 27 mai 2023, 19:35, à 19:35, t-w @.***> a écrit:

  1. Changed way of defining the mode in adfFileOpen, the 2 modes replace as follows:

    • "r" -> ADF_FILE_MODE_READ
    • "w" -> ADF_FILE_MODE_READWRTIE
  2. Added a few useful functions (very simple - static inline):

    • for checking status (position, size)
    • checking EOF changed also to static inline for consistency
    • seeking (start, EOF)

You can view, comment on, or merge this pull request online at:

https://github.com/lclevy/ADFlib/pull/47

-- Commit Summary --

  • adf_file: new useful (inline) functions for checking status: position, size; checking EOF also changes to inline.
  • adffile: adding 2 (inline) function for seeking to the start and end of the file (local/static functions, used internally renamed with "" as postfix).
  • adfFileOpen: open mode definitions changed from strings to ADFlib-specific enums (to avoid confusion with other APIs).

-- File Changes --

M examples/unadf.c (2) M regtests/Test/access.c (2) M regtests/Test/comment.c (2) M regtests/Test/dispsect.c (2) M regtests/Test/file_read_hard_link_test.c (3) M regtests/Test/file_seek_after_write.c (10) M regtests/Test/file_seek_test.c (4) M regtests/Test/file_seek_test2.c (3) M regtests/Test/file_test.c (6) M regtests/Test/file_test2.c (4) M regtests/Test/file_test3.c (4) M regtests/Test/floppy_overfilling_test.c (4) M regtests/Test/rename.c (10) M regtests/Test/undel.c (2) M regtests/Test/undel2.c (2) M regtests/Test/undel3.c (2) M src/adf_file.c (66) M src/adf_file.h (33) M tests/test_file_append.c (44) M tests/test_file_create.c (10) M tests/test_file_overwrite.c (18) M tests/test_file_overwrite2.c (16) M tests/test_file_seek.c (8) M tests/test_file_seek_after_write.c (6) M tests/test_file_truncate.c (18) M tests/test_file_truncate2.c (14) M tests/test_file_write.c (14) M tests/test_file_write_chunks.c (14) M tests/test_util.c (4)

-- Patch Links --

https://github.com/lclevy/ADFlib/pull/47.patch https://github.com/lclevy/ADFlib/pull/47.diff

-- Reply to this email directly or view it on GitHub: https://github.com/lclevy/ADFlib/pull/47 You are receiving this because you are subscribed to this thread.

Message ID: @.***>

kyz commented 1 year ago

Question here:

I was thinking about changing struct AdfFile ... to typedef struct AdfFile {...} AdfFile. This would simplify using the interface (struct everywhere adds unnecessary clutter, it is clear the this is a custom type).

But probably the same would have to be applied to other modules for consistency (either now, for 0.8.0, or later - for me both are fine).

What do you think?

This is a matter of style. and it really depends on the project. Some projects go all in with it, others don't use it at all.

Personally, I don't like it, I want bare structs, because it makes it clear what you are manipulating, and it stops the code looking too much like C++. Having to type "struct" is a feature, not a bug, you never make the mistake of thinking what you have is not a struct. I prefer typedef only for ints, enums, where there is a meaningful difference between two otherwise identical types (e.g. time_t is not the same thing as off_t). structs don't have that issue.

t-w commented 1 year ago

Question here: I was thinking about changing struct AdfFile ... to typedef struct AdfFile {...} AdfFile. This would simplify using the interface (struct everywhere adds unnecessary clutter, it is clear the this is a custom type). But probably the same would have to be applied to other modules for consistency (either now, for 0.8.0, or later - for me both are fine). What do you think?

This is a matter of style. and it really depends on the project. Some projects go all in with it, others don't use it at all.

Exactly. That's why I ask, what are in general preferences here.

Personally, I don't like it, I want bare structs, because it makes it clear what you are manipulating, and it stops the code looking too much like C++.

Fair point.

IMHO, typedef in such cases does not hide much, though. Nearly nothing will compile if you confuse a struct with some basic type and it still gives you rather strong type checking. On the contrary, aliasing eg. int types with typedef, having stdint.h and stdbool.h, in many cases rather confuses than helps.

For me, typedef in C is mostly for the above. And without it, I find very little use for it...

Having to type "struct" is a feature, not a bug, you never make the mistake of thinking what you have is not a struct. I prefer typedef only for ints, enums, where there is a meaningful difference between two otherwise identical types (e.g. time_t is not the same thing as off_t). structs don't have that issue.

OK. I see your point. This is indeed more C-style. And for some cases, it does make sense.

I find certain C-style practices inherently bad, though... The most prominent is the persistent lack of proper type qualifications, especially in the interfaces (even in standard libraries), which results in:

  1. lack of proper constraints that help a lot to limit bugs inside
  2. gives trouble if you want to make something const, but have to use a function, clearly using the object "read-only" but gets the thing without const - what enforces user code to drop const, what results in even less constraints, this time in user code...

And adding proper type qualifications makes the interface definitions bloated, but, at the same time, they start carrying a lot of constraints and information (which is good).

That is why I, personally, prefer to typedef structs, which, IMHO, do not really help in code readability.

For ADFlib, I would propose, eventually, to typedef only those main structs: AdfDevice, AdfVolume, AdfFile, AdfEntry - which are used a lot in external code and, indeed, are in a way C++-like objects with their "methods". Other structs (blocks and some other internal ones) could be left with struct.

But, as you wrote, this is a matter of style. There would be a lot to change to apply this, so if it is preferred to keep the style, I will not modify it then. We leave it as it is.