Re: [RFC] [WIP] incorporate picotcp into barebox: a small demo

[Antony Pavlov <antonynpavlov@gmail.com>]

On Tue, 27 May 2014 11:46:29 +0200
Daniele Lacamera <daniele.lacamera@tass.be> wrote:

> Antony, regarding your comments on picotcp "cleaning", could you
> please elaborate a bit more, taking into account my comments below?
> 
> On Mon, May 26, 2014 at 2:09 PM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> > Picotcp need some work for cleaning. I think that on average barebox code is more clean
> > that picotcp code (there are too many #ifdefs, some compiler warnings, formatting),
> > but IMHO it is not very difficult to make it cleaner.
> >
> 
> - The amount of #ifdefs in the code is due to the size optimizations.
> Types are left out when modules are disabled, would be much more
> difficult to achieve the same code size, e.g. on a 8-bit machine, if
> we used empty proxies instead, like for instance Linux does. Keep in
> mind that saving a few bytes is the key for some of our projects, so I
> would accept no modification for the sack of aesthetic if it would
> impact on code size. Our quality processes ensure that the branching
> is kept under control, and our continuous integration takes into
> account enabling and disabling the modules.

linux kernel and barebox use not so much #ifdef's because they use
they use IS_ENABLED() macro with just the same result.

Please see linux.git/drivers/gpu/drm/tegra/dsi.c
(https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/tegra/dsi.c#L628):

        if (IS_ENABLED(CONFIG_DEBUG_FS)) {
                err = tegra_dsi_debugfs_init(dsi, tegra->drm->primary);
                if (err < 0)
                        dev_err(dsi->dev, "debugfs setup failed: %d\n", err);
        }

so the code under IS_ENABLED() is always parsed by C compiler;
if there is an syntax error in this code then the compilation will be stopped.

In the examople above, if CONFIG_DEBUG_FS is not activated then
clever optimizing compiler will drop all code under IS_ENABLED().

If you use #ifdef:

        #ifdef CONFIG_DEBUG_FS
                err = tegra_dsi_debugfs_init(dsi, tegra->drm->primary);
                if (err < 0)
                        dev_err(dsi->dev, "debugfs setup failed: %d\n", err);
        #endif
   
then you get jut the same result but without checking the code under #ifdef CONFIG_DEBUG_FS.

> - compiler warnings: AFAIK, our code is warning free on gcc, it might
> be that a specific platform config could trigger some. We are
> interested about your experience, please share your findings, but
> please do not report -Wshadow warnings obtained with broken gcc (<=
> 4.6).
> Our default set of warning flags:  -Wall -Wdeclaration-after-statement
> -W -Wextra -Wshadow -Wcast-qual -Wwrite-strings
> -Wmissing-field-initializers -Wconversion -Wcast-align

$ gcc --version
gcc (Debian 4.8.3-1) 4.8.3

Here is my 'make -s' output for sandbox barebox:

net/picotcp/modules/pico_ipv4.c: In function ‘pico_ipv4_rebound_large’:
net/picotcp/modules/pico_ipv4.c:1582:24: warning: unused variable ‘fr’ [-Wunused-variable]
     struct pico_frame *fr;
                        ^
net/picotcp/modules/pico_ipv4.c:1581:14: warning: unused variable ‘len’ [-Wunused-variable]
     uint32_t len = f->transport_len;
              ^
net/picotcp/modules/pico_ipv4.c:1580:14: warning: unused variable ‘total_payload_written’ [-Wunused-variable]
     uint32_t total_payload_written = 0;
              ^
net/picotcp/stack/pico_socket.c: In function ‘pico_socket_add’:
net/picotcp/stack/pico_socket.c:397:5: warning: "DEBUG_SOCKET_TREE" is not defined [-Wundef]
 #if DEBUG_SOCKET_TREE
     ^
net/picotcp/stack/pico_socket.c: At top level:
net/picotcp/stack/pico_socket.c:1037:13: warning: ‘pico_socket_xmit_first_fragment_setup’ defined but not used [-Wunused-function]
 static void pico_socket_xmit_first_fragment_setup(struct pico_frame *f, int space, int hdr_offset)
             ^
net/picotcp/stack/pico_socket.c:1049:13: warning: ‘pico_socket_xmit_next_fragment_setup’ defined but not used [-Wunused-function]
 static void pico_socket_xmit_next_fragment_setup(struct pico_frame *f, int hdr_offset, int total_payload_written, int len)
             ^
net/test_picotcp.c: In function ‘do_test_ipv4’:
net/test_picotcp.c:51:48: warning: comparison between pointer and integer [enabled by default]
         fail_unless(pico_ipv4_link_find(&a[i]) == dev[i], "Error finding link");
                                                ^
net/test_picotcp.c:11:6: note: in definition of macro ‘fail_if’
  if (a) { \
      ^
net/test_picotcp.c:51:9: note: in expansion of macro ‘fail_unless’
         fail_unless(pico_ipv4_link_find(&a[i]) == dev[i], "Error finding link");
         ^

> 
> - formatting: Could it be that you checked an intermediate
> masterbranch version? We run uncrustify periodically on the code, and
> our formatting is consistent with our own rules.


Yes, I see your 'Enforced coding style via uncrustify' commit.

But in linux kernel, barebox, qemu and IMHO many other projects
there are:

  * CODING_STYLE documentation file;
  * scripts/checkpatch.pl

The scripts/checkpatch.pl is used by project maintainer on git patches
before applying them, so there is very small possibility for
bad formatted patches to penetrate into repository.

There is even cruel practice running checkpatch.pl script via git pre-commit
hook so one can't even locally commit bad formatted changes!
(see http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html)


-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox