From: arne@vajhoej.dk
On 7/3/2025 9:00 PM, Craig A. Berry wrote:
>
> On 7/3/25 7:24 PM, Arne Vajhøj wrote:
>> On 7/3/2025 6:31 PM, Craig A. Berry wrote:
>>> https://github.com/FreeTDS/freetds/blob/Branch-1_5/src/replacements/
>>> vasprintf.c
>>>
>>> does the following:
>>>
>>> #if HAVE_PATHS_H
>>> #include
>>> #endif /* HAVE_PATHS_H */
>>> . . .
>>> #ifndef _PATH_DEVNULL
>>> #define _PATH_DEVNULL "/dev/null"
>>> #endif
>>>
>>> FreeTDS is relying on autoconf to sort out whether paths.h is available
>>> and will only include it if HAVE_PATHS_H is defined in config.h, and
>>> thus only defines _PATH_DEVNULL if it's known not to exist. Obviously
>>> one should avoid making up one's own reserved identifiers and clobbering
>>> documented and well-known names, but that's not really the same thing as
>>> providing an implementation for one that's known to be missing. This
>>> fallback approach could of course be added to Arne's implementation if
>>> desired; to me it seems a lot cleaner than defining yet another macro,
>>> which would need to be defined in terms of _PATH_DEVNULL when it exists
>>> but otherwise not.
>>>
>>> The problem for VMS is that the workaround doesn't work. Or at least it
>>> didn't when I was first porting FreeTDS 20+ years ago because the CRTL
>>> at the time did not recognize '/dev/null' as a valid path. I believe
>>> recentish CRTLs (maybe VMS 8.x and later?) do have special case code
>>> that translates it to the native null device. But that wasn't
>>> available, so I added the following line to the template from which
>>> config.h is generated on VMS:
>>>
>>> #define _PATH_DEVNULL "_NLA0:"
>>>
>>> The main advantage of this approach is that it worked, and it did so
>>> with a one-line change to a file I was already maintaining. I didn't
>>> have to sprinkle '#ifdef __VMS' in files that already existed and were
>>> already working on other platforms. I didn't have to create a lot of new
>>> files, the presence of which in the repository I couldn't test without
>>> autoconf/automake, which I didn't have access to at the time. All of
>>> which meant upstream maintainers were far more likely to accept my
>>> changes.
>>
>> So you are saying that for the code to work on *nix then I need
>> to keep those, because having it defined in config.h is a non-*nix
>> thing and for *nix it only set HAVE_PATHS_H which requires the
>> include in this file?
>>
>> I will update.
>
> I think preserving the HAVE_PATHS_H check is probably a good idea. It
> makes sure you get the correct, system-specific version of
> _PATH_DEVNULL, if any. And it preserves an existing approach to
> portability that you aren't worried about so you can focus on the
> vasprintf() guts.
I have updated.
> I haven't done any testing of your code. I do agree with Dan that
> reducing the repeated code would be a benefit. You don't need the first
> #if because the file won't even be built if HAS_VASPRINTF is detected
> during configuration.
I know. But just in case it did get called.
> Which means for the remaining 4 cases you've got
> this in all of them that could be put outside the #ifdefs, and without
> any nesting:
>
> va_list cp;
> va_copy(cp, ap);
>
> (len < 0) return len;
>
> *ret = malloc(len + 1);
> if(!*ret) return -1;
> return vsprintf(*ret, fmt, cp);
>
> where something1 is always 1-3 lines and something2 is zero or one line
There are duplicated lines.
But I still believe the one block of code per case is more readable
than common code + some case specific code + common code + some
case specific code + common code.
Also imagine if someone was to add yet another approach. Now it
is just put in the #if for the case and then write the block of
code. If mixing it would all depend on the new approach - maybe
it would fit into the existing structure, maybe it would require
changing the structure if it does not fit.
Arne
--- SoupGate-Win32 v1.05
* Origin: you cannot sedate... all the things you hate (1:229/2)
|
