bug-datamash
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Patches for --vnlog support


From: Tim Rice
Subject: Re: Patches for --vnlog support
Date: Fri, 5 Aug 2022 23:09:07 +0000

Hey Dima,

I'm looking at the new _line_record_fread function and comparing it to the 
wrapper functions line_record_fread and line_record_fread_vnlog_prologue. I 
wonder if that could be simpler.

It seems like the `vnlog_prologue` argument to the 
line_record_fread_vnlog_prologue is only used to turn on code which is already 
being turned on only if the vnlog variable is already true.

I.e. this part:

```
_line_record_fread (struct /* in/out */ line_record_t* lr,
                    FILE *stream, char delimiter,
                    bool skip_comments,
                    bool vnlog_prologue)
{
...
    if( vnlog )
    {
        if( vnlog_prologue )
        {
```

Rather than making a new function with two wrapper functions, why don't we just 
add a `if( vnlog )` toggle inside the existing `line_record_fread` function?

Or are there scenarios where vnlog will be true but vnlog_prologue could turn 
out to be false?


Even if we do need the vnlog_prologue toggle inside that section of code, there 
are a couple of alternative ways to do it which I might prefer:

* We could make the existing line_record_fread variadic, so toggling 
vnlog_prologue would merely be an optional extra argument to the existing 
function.

* We could use function pointers, to avoid all the `if( (!vnlog && 
line_record_fread(...)) || ( vnlog && line_record_fread_vnlog_prologue(...)))` logic 
which has appeared. Simply set the old line_record_fread as default, and override it with the 
alternative function during option processing.

Thoughts?

~ Tim



reply via email to

[Prev in Thread] Current Thread [Next in Thread]