[FFmpeg-devel] [PATCH v7 2/3] avformat/os_support: Support long file names on Windows

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of nil-
> admirari at mailo.com
> Sent: Wednesday, May 25, 2022 9:17 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v7 2/3] avformat/os_support: Support
> long file names on Windows
> 
> > No, it is intended and expected that the structs are different.
> > ...
> > That's why the structs are different and the fields of
> > win32_stat always large enough, no matter which struct
> > is being used internally.
> 
> Please document that there is a potential difference in time types
> and that the difference is intentional, and that the chosen
> time type is always large enough.
> 
> Probably it's worthwhile to document that the entire machinery was created
> because of POSIX stat function and struct being identically named,
> which is not possible to accommodate by a simple macro.

Yes, it makes sense to explain that a bit. Will do.


> > > > +static inline int win32_access(const char *filename_utf8, int par)
> > > > +static inline void copy_stat(struct _stati64 *winstat, struct
> > > win32_stat *par)
> > > > +static inline int win32_stat(const char *filename_utf8, struct
> > > win32_stat *par)
> > > > +static inline int win32_fstat(int fd, struct win32_stat *par)
> > >
> > > How about renaming par to something more appropriate?
> >
> > How? And why?
> >
> > These functions were always named like that (it just wasn't visible
> > as these were constructed through macros).
> 
> _access argument is called mode:
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/access-
> waccess?view=msvc-170.
> _stat and _fstat argument is called a buffer:
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/stat-
> functions?view=msvc-170
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/fstat-
> fstat32-fstat64-fstati64-fstat32i64-fstat64i32?view=msvc-170
> It's no better than par, but you don't have to follow MS.

I misread, thinking you wanted to rename the functions. Yes, the par
comes from the macro (which was for building the two functions with 
different parameter name). I have expanded the macro for stat, 
and then the macro would have only existed for access, so I did 
expand that one as well.

So, no problem, I will rename the arguments according to posix.

> Somehow winstat refers to parameters of type _stati64, not win32_stat.

I named it winstat for "win api stat". When you want to go strict
you could say that all the naming is incorrect, because 'win32' 
suggests that all those are about calls to the Win32 API, while
in fact, they are calls to msvcrt.

> I would've called win32_stat params winstat, and _stati64 params crtstat,
> but you can always come up with better names.

win32_stat has nothing to do with Windows, it is actually more a posix_stat,
but it needs to have the same name as the function

Even crtstat would not be quite correct and would rather need to 
be named msvcrtstat :-)

We could drive this further and further and probably it would never
be totally "right". Though, I will make the replacements you asked for,
and then we'll see...


Regarding your concerns about _USE_32BIT_TIME_T, I wanted to mention
that we still have an alternative, to get around this.

Currently, we are using _wstati64, _ffstati64 and _stati64. All of 
those are re-defined depending on whether _USE_32BIT_TIME_T is
defined or not.

Instead of that, we could use _wstat64, _ffstat64 and _stat64. In 
this case, we would always get 64bit time values, independent
of the definition of _USE_32BIT_TIME_T.

The only difference it makes would be whether we can have file times
beyond the year 2038.

I'm fine with the way it is right now, I just wanted to have mentioned
it.

Thanks for reviewing,
softworkz