Hi,

I'm just returning from a debugging session of an error that in my eyes was caused by a badly implemented File.seek() from Phobos, which has the signature

  /**
    Calls $(HTTP cplusplus.com/reference/clibrary/cstdio/fseek.html, fseek)
    for the file handle.

    Throws: `Exception` if the file is not opened.
            `ErrnoException` if the call to `fseek` fails.
  */
  void seek(long offset, int origin = SEEK_SET) @trusted
  { /*...*/ }

Unfamiliar with the ins and outs of the C stdlib, and in absence if sufficient Phobos documentation, I assumed origin to be a position in bytes. I need the offset from the current position, so I used a call to File.tell() in place of origin. I assumed SEEK_SET to be defined as a negative number. My program terminated without any message as soon as origin == 4 (on Windows).

I now know that origin MUST be any of SEEK_SET, SEEK_CUR or SEEK_END, and nothing else. These are defined in core.stdc.stdio, but they aren't linked to the Phobos documentation of seek.

My misunderstanding is caused by Phobos unnecessarily propagating the shortcomings of C into its API.

I am prepared to contribute a PR to fix this, please let me know if my plan has any holes:

1) Change the enum containing SEEK_* to a named enum,
2) Change the type of the origin parameter to this enum.
3) Document.

Would we need to keep the original signature around as an overload to prevent code breakage? Maybe not? But if so, would that work without conflicts?

Thanks,
Bastiaan.

On Monday, 8 April 2019 at 13:56:24 UTC, Bastiaan Veelo wrote:
> 1) Change the enum containing SEEK_* to a named enum,
> 2) Change the type of the origin parameter to this enum.
> 3) Document.
>
> Would we need to keep the original signature around as an overload to prevent code breakage? Maybe not? But if so, would that work without conflicts?

I don't think an overload is necessary. I am thinking along these lines [1]

import std.stdio;

enum SEEK
{
    /// Offset is relative to the beginning
    SET,
    /// Offset is relative to the current position
    CUR,
    /// Offset is relative to the end
    END
}
enum SEEK_SET = SEEK.SET;
enum SEEK_CUR = SEEK.CUR;
enum SEEK_END = SEEK.END;

void seek(long offset, int origin = SEEK_SET) {writeln("orig");} // Original, not called anymore.
void seek(long offset, SEEK origin = SEEK.SET) {writeln("new");}

void main()
{
    seek(0, SEEK_CUR); // Legacy
    seek(0, SEEK.CUR); // Recommended
}

[1] https://run.dlang.io/is/MllZ5Q

On Monday, 8 April 2019 at 14:49:06 UTC, Bastiaan Veelo wrote:
> On Monday, 8 April 2019 at 13:56:24 UTC, Bastiaan Veelo wrote:
>> 1) Change the enum containing SEEK_* to a named enum,
>> 2) Change the type of the origin parameter to this enum.
>> 3) Document.
>>
>> Would we need to keep the original signature around as an overload to prevent code breakage? Maybe not? But if so, would that work without conflicts?
>
> I don't think an overload is necessary. I am thinking along these lines [1]
>
> import std.stdio;
>
> enum SEEK
> {
>     /// Offset is relative to the beginning
>     SET,
>     /// Offset is relative to the current position
>     CUR,
>     /// Offset is relative to the end
>     END
> }
> enum SEEK_SET = SEEK.SET;
> enum SEEK_CUR = SEEK.CUR;
> enum SEEK_END = SEEK.END;
>
> void seek(long offset, int origin = SEEK_SET) {writeln("orig");} // Original, not called anymore.
> void seek(long offset, SEEK origin = SEEK.SET) {writeln("new");}
>
> void main()
> {
>     seek(0, SEEK_CUR); // Legacy
>     seek(0, SEEK.CUR); // Recommended
> }
>
> [1] https://run.dlang.io/is/MllZ5Q

You should just have created a PR in phobos and left discussion within there instead of here.