Remove implementation-specific inline functions with exactly one caller?

[Joel Sherrill]

On Tue, Oct 13, 2020 at 6:41 AM Sebastian Huber <
sebastian.huber at embedded-brains.de> wrote:

> Hello,
>
> we have a couple of implementation-specific inline functions defined in
> header files which have exactly one caller. I think we should remove
> these functions in general, to reduce the lines of code, making code
> review easier, and reduce the number of functions which need
> documentation. If it makes the caller site more clear, then we should
> place them in the source file of the caller. Examples:
>
> cpukit/rtems/src/partcreate.c:  the_partition = _Partition_Allocate();
> cpukit/include/rtems/rtems/partimpl.h:RTEMS_INLINE_ROUTINE
> Partition_Control *_Partition_Allocate ( void )
>
> cpukit/include/rtems/rtems/regionimpl.h:RTEMS_INLINE_ROUTINE bool
> _Region_Free_segment (
> cpukit/rtems/src/regionreturnsegment.c:  if ( _Region_Free_segment(
> the_region, segment ) ) {
>
> cpukit/sapi/src/extensiondelete.c:  the_extension = _Extension_Get( id );
> cpukit/include/rtems/extensionimpl.h:RTEMS_INLINE_ROUTINE
> Extension_Control *_Extension_Get( Objects_Id id )
>
> cpukit/sapi/src/extensioncreate.c:  the_extension = _Extension_Allocate();
> cpukit/include/rtems/extensionimpl.h:RTEMS_INLINE_ROUTINE
> Extension_Control *_Extension_Allocate( void )
>
> cpukit/sapi/src/extensiondelete.c:    _Extension_Free( the_extension );
> cpukit/include/rtems/extensionimpl.h:RTEMS_INLINE_ROUTINE void
> _Extension_Free (
>
> What do you think?
>

I think that having a single caller is not by itself a reason to eliminate
a method.

Although it predates C++, the design of RTEMS internal interfaces was based
on strong packaging and encapsulation.

This particular change would expose the allocation scheme for these object
classes to the caller and the need for a cast. Under the original rules,
hiding
that had value.

In this case, it is probably OK because the caller should always be the API
 that actually defined the method  But if the simple, single caller method
is not defined in the same "class" where it is called, that would break the
encapsulation.

We also have to consider if this has a negative impact on readability. In
this
case, that seems unlikely as well since the verb isn't changing and there
is no algorithm or internals exposure. Sometimes, a simple inline is hiding
something that is ugly and makes the callers look better. Certainly this
is hiding a cast. But that's not necessarily inherently ugly.

I'm not giving blanket permission. I'm saying it is worth considering on a
case by case method and updating the coding conventions on inlines.

--joel

>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20201013/7b177754/attachment.html>