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

[j***@gmail.com]

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.

[Tanu Kaskinen]

Post by j***@gmail.com
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()?

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.

[Tanu Kaskinen]

Post by Tanu Kaskinen

Post by j***@gmail.com
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()?
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.

I applied the rttime patches as well now.

[j***@gmail.com]

Post by Tanu Kaskinen

Post by j***@gmail.com
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. :)

Post by Tanu Kaskinen
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 :)

[Tanu Kaskinen]

Post by j***@gmail.com

Post by Tanu Kaskinen

Post by j***@gmail.com
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.

[j***@gmail.com]

Post by Tanu Kaskinen

Post by j***@gmail.com

Post by Tanu Kaskinen

Post by j***@gmail.com
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.

Post by Tanu Kaskinen
Anyway, I'll send the constification patch.

Ok :)

[Tanu Kaskinen]

Post by j***@gmail.com

Post by Tanu Kaskinen
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.

Post by Tanu Kaskinen
Anyway, I'll send the constification patch.

Ok :)

I applied my patch now, and the rest of your patches. Thanks for the
patches!

[j***@gmail.com]

Post by Tanu Kaskinen

Post by j***@gmail.com

Post by Tanu Kaskinen
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.

Post by Tanu Kaskinen
Anyway, I'll send the constification patch.

Ok :)

I applied my patch now, and the rest of your patches. Thanks for the
patches!

Ok, no problem :)