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