On Tue, Mar 19, 2019 at 2:33 PM Jeff Hostetler <git@xxxxxxxxxxxxxxxxx> wrote:
> On 3/19/2019 12:42 PM, Eric Sunshine wrote:
> > Rather than adding a new "sparse" mode, I wonder if this entire
> > concept can be boiled down to a single new function:
> >
> > void finish_progress(struct progress **p_progress, const char *msg)
> > {
> > [...]
> > }
> >
> > without having to make any other changes to the implementation or add
> > a new field to the structure.
>
> The existing model has start_progress() and start_delayed_progress().
> I was following that model and added the new option at the start.
> I'm not planning on adding any additional flags, but if we had some
> we'd want them available on the startup next to this one.
Perhaps it makes sense to just take a 'flags' argument instead of
'sparse' so we don't have to worry about this going forward. In other
words:
#define PROGRESS_DELAYED (1 << 0)
#define PROGRESS_SPARSE (1 << 1)
struct progress *start_progress_x(const char *title, uint64_t total,
unsigned flags);
which covers "delayed" start and "sparse". ("_x" is a placeholder
since I can't think of a good name).
> > It would mean, though, that the caller
> > would have to remember to invoke finish_progress() rather than
> > stop_progress().
>
> Right, I was trying to isolate the change to the setup, so that loop
> bottoms and any in-loop return points don't all have to worry about
> whether to call stop_ or finish_.
Yes, I understand the benefit.
Anyhow, my comments are akin to bikeshedding, thus not necessarily actionable.
