New subject: Semantic Patching (was Re: Current osmo-pcu patch set)

21 Sep 2017

Hi,

I am a bit lost with the review of the current osmo-pcu patchset. Going through
the patches I have difficulties to see the red thread and what is better after
we integrate the changes.

I guess the goal is to add the allocation of multiple uplink timeslots but from
my point of view this is buried in a lot of subjective sideway development. My
wish would be:

For multiple slot UL allocation:

Make the most direct patchset to implement this feature. I understand that to
avoid code duplication some methods can be re-used but instead of having 10
patches that change int->bool, change signatures of functions, maybe make it
minimal to see how it works first?

After it works send a cover mail that explains what and why you took the
approach, what the _speed_ improvement for browsing is or when it is being
used. What you see the danger in terms of common ts and allocation.

For the clean-ups:

I don't say we shouldn't do them but we should do them in a way they don't
take much time to review and still provide a net gain to the contributor.
A "cosmetic" patch that breaks the build is very suspicious and forces a
reviewer to look extra carefully into these "clean-ups".

int->bool:

I think int->bool and 1->true adds little value for an application but if
it is important then please do it by semantic patching so they can be easily
reviewed.

bts->trx:

I think there is little value to change a function signature from getting
a BTS to a TRX when the implementation starts to do:

	- bts->list
	+ trx->bts->list

"API" documentation:

Comments can be helpful and it is good to explain on a high level what a
function will do and what the side-effects will be. But in an application
I think writing:

	\param[in,out] bts Pointer to BTS struct

adds not enough value. First it is wrong as the type is not "BTS" but it
is the "C" gprs_rlcmac_bts. And second doxygen will perfectly tell you the
type of of the parameter anyway.

Documenting that -1 of a parameter has a specific semantic can have some
advantage but the risk of code/doc not agreeing is there as well.

libosmocore:

We do have a desire for reuse but everytime you make osmo-pcu depend on a
unreleased version of libosmocore rebuilding osmo-pcu get's more difficult
and the compile error is not obvious. It is not clear where the macro is
coming from. So please keep this in mind and try to find a balance.

thank you for your consideration

	holger