[pulseaudio-discuss] [patches] constification round #3

[jnqnfe at gmail.com]

On Mon, 2018-06-18 at 10:47 +0300, Tanu Kaskinen wrote:
> On Fri, 2018-06-15 at 21:34 +0100, jnqnfe at gmail.com wrote:
> > On Fri, 2018-06-15 at 14:15 +0300, Tanu Kaskinen wrote:
> > > On Thu, 2018-06-07 at 05:01 +0100, jnqnfe at gmail.com wrote:
> > > > API constification set #3
> > > > 
> > > > Some API functions perform validation routines which may modify
> > > > the
> > > > 'error' attribute of a context object. For API functions where
> > > > the
> > > > "primary" object is not a context object, and the object holds
> > > > a
> > > > non-
> > > > const context pointer, this internal mechanism could be
> > > > successfully
> > > > hidden - such functions were constified in my previous two
> > > > patch
> > > > sets.
> > > > 
> > > > However, some functions, namely those related directly to
> > > > context
> > > > objects, had to be passed over previously. These could not be
> > > > constified because the validation routines accessed the context
> > > > object
> > > > directly to change the error attribute.
> > > > 
> > > > This patch set addresses this shortcoming.
> > > > 
> > > > Firstly the patch set moves the context's error attribute
> > > > behind a
> > > > pointer. The validation routines are then changed to store the
> > > > error
> > > > value via this pointer. That then allows a collection of
> > > > further
> > > > API
> > > > functions to be constified.
> > > > 
> > > > (actually pa_context_errno and the two rttime ones could have
> > > > been
> > > > done
> > > > previously on second inspection)
> > > > 
> > > > Although the indirection of the error attribute is obviously
> > > > ever
> > > > so
> > > > slightly worse off for efficiency, it is surely worth the
> > > > price.
> > > > After
> > > > all, the error setting of the validation checks is just an
> > > > artifact
> > > > of
> > > > an internal mechanism and should not be allowed to influence
> > > > the
> > > > public
> > > > API like it currently does.
> > > 
> > > Thanks! I applied patches 1 and 4. Patches 2 and 3 seem
> > > unnecessarily
> > > complicated, wouldn't it be better to just constify
> > > pa_context_set_error()?
> > 
> > It would certainly be more simple, but I would say that it comes
> > down
> > to perceived purpose of a `pa_context_set_error` function.
> > Logically
> > any and all `pa_context_set_*` functions should be expected to
> > mutate
> > the context object in some way and thus take a non-const pointer.
> > Diverging from this in general risks confusion (even though it's
> > internal only), and as a rule I'd err away from making such a
> > function
> > immutable just because an implementation detail allows you to,
> > that's
> > why I went down the route I did.
> > 
> > On the other hand if this function were renamed to something that
> > sufficiently conveyed a 'modify internal mutable component of
> > otherwise
> > immutable object' purpose, with an explanation in the
> > documentation,
> > then that could be perfectly sufficient (perhaps
> > `pa_context_set_internal_error` would do), but then you're risking
> > pushing implementation detail (via the name) out to every place in
> > the
> > library that wants to set an error value, not just the validation
> > routines. Is that acceptable?
> > 
> > Ultimately though it's not at all important to me, I'm happy for
> > you
> > guys to go whatever way you decide and I'm not going to kick up a
> > fuss
> > if you do what you suggest. :)
> 
> Your patches are based on the view that setting the error code isn't
> considered mutating the context object. I think constifying
> pa_context_set_error() is fully in line with that view. Or do you
> think
> that when the validation macros set the code, that's not mutating the
> context object, but when other code calls pa_context_set_error(),
> that
> is mutating the context object?

Yes, the latter.

> Anyway, I'll send the constification patch.

Ok :)