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

[Tanu Kaskinen]

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?

Anyway, I'll send the constification patch.

-- 
Tanu

https://liberapay.com/tanuk
https://www.patreon.com/tanuk