www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - File.seek() offset parameter should be enum, not int.

reply Bastiaan Veelo <Bastiaan Veelo.net> writes:
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.
Apr 08 2019
parent reply Bastiaan Veelo <Bastiaan Veelo.net> writes:
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
Apr 08 2019
parent bauss <jj_1337 live.dk> writes:
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.
Apr 08 2019