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
