[U-Boot] [PATCH 0/2] Coding style to avoid #ifdef everywhere

* [U-Boot] [PATCH 0/2] Coding style to avoid #ifdef everywhere
@ 2014-02-13  9:30 Masahiro Yamada
  2014-02-13  9:30 ` [U-Boot] [PATCH 1/2] Move #ifdef(CONFIG_DISPLAY_CPUINFO) from caller to callee Masahiro Yamada
  2014-02-13  9:30 ` [U-Boot] [PATCH 2/2] Move CONFIG_DISPLAY_CPUINFO to Makefile Masahiro Yamada
  0 siblings, 2 replies; 5+ messages in thread
From: Masahiro Yamada @ 2014-02-13  9:30 UTC (permalink / raw)
  To: u-boot

I agree that many parts of U-Boot code are
sprinkled with #ifdef.

But I am opposed to adding a gimmick to conceal ugly code.
(Rather, we should fix code correctly.)

I guess most of U-Boot developers are working on Linux, too.
But I am afraid U-Boot code is dirty compared with Kernel
especially when it comes to the usage of #ifdef.

I think we all should keep in mind the basic coding style
often used in Linux Kernel.

[1] Unpleasant Coding Style
    ^^^^^^^^^^

In this coding style, #ifdef .. #endif
appear on both callee and caller.

The func definition part is surrounded by #ifdef
like this:

  #ifdef CONFIG_FOO
  int foo_init(struct foo_struct *foo)
  {
        blah blah
        blah blah
  }
  #endif

And the caller is like this:

  int caller_of_foo(void)
  {
  #ifdef
            int error;
  #endif

       blah blah blah

  #ifdef CONFIG_FOO
           error = foo_init(&foo);
           if (error) {
                 printf("Foo error\n");
                 return error;
           }
  #endif

      blah blah blah

  }

If we adopt this style, we must #ifdef
variable declarations as well as the code that uses them.

One funtion is generally called from multiple places.
So we need to add #ifdef to all code which invokes the function.
As a result, we will have code sprinkled with #ifdefs.

[2] Pleasant Coding Style (often used in Linux Kernel.)

In this coding style, #ifdef appear only on the definition and the
prototype.

The definition part is like this:

  #ifdef CONFIG_FOO
  int foo_init(struct foo_struct *foo)
  {
        blah blah
        blah blah
  }
  #endif

(Or, in Makefile
obj-$(CONFIG_FOO) += foo_init.o  )

The prototype in a header file is like this:

  #ifdef CONFIG_FOO
  int foo_init(struct foo_struct *foo);
  #else
  static inline foo_init(struct foo_struct *foo)
  {
          return 0;
  }
  #endif

And the caller is like this:

  int caller_of_foo(void)
  {
        int error;

         blah blah blah

         error = foo_init(&foo);
         if (error) {
               printf("Foo error\n");
               return error;
         }

         blah blah blah

   }

In this style, we don't #ifdef in the caller.

If CONFIG_FOO is not defined, the function is
defined as a static inline function returning a constant value.
We generally expect it will be removed (or quite simplified)
by the compiler optimization.
This is better than weak attribute.
(Weak attribute makes it difficult to track which definition is used.)

Using #ifdef in this style will get over our bad habit:
We are too dependent on garbage collection. (--gc-sections option)
We should compile only what we need in the first place.

This series includes simple examples how we should fix the code.

Comments are welcome.

Masahiro Yamada (2):
  Move #ifdef(CONFIG_DISPLAY_CPUINFO) from caller to callee
  Move CONFIG_DISPLAY_CPUINFO to Makefile

 arch/arm/cpu/arm926ejs/omap/Makefile           | 3 ++-
 arch/arm/cpu/arm926ejs/omap/cpuinfo.c          | 4 ++--
 arch/arm/cpu/armv7/omap-common/hwinit-common.c | 3 +++
 arch/arm/cpu/tegra-common/Makefile             | 3 ++-
 arch/arm/cpu/tegra-common/sys_info.c           | 2 --
 arch/arm/include/asm/arch-am33xx/sys_proto.h   | 4 ----
 arch/arm/lib/board.c                           | 4 ----
 board/altera/socfpga/socfpga_cyclone5.c        | 2 ++
 board/freescale/mx53loco/mx53loco.c            | 2 ++
 board/nokia/rx51/rx51.h                        | 2 --
 common/board_f.c                               | 2 --
 include/common.h                               | 7 +++++++
 12 files changed, 20 insertions(+), 18 deletions(-)

-- 
1.8.3.2

^ permalink raw reply	[flat|nested] 5+ messages in thread