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

[jnqnfe at gmail.com]

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. :)

> I altered the commit message of patch 1, because "See email
> discussion"
> seemed like something that could be very annoying to read in the
> commit
> message e.g. ten years from now. I replaced it with a short summary
> of
> the justification for the change.

Yeah, I agree it can be annoying, no problem :)