Proliferation of hundreds of tiny files

[Dmitry V. Levin]

On Fri, Oct 23, 2015 at 09:53:02AM +0200, Denys Vlasenko wrote:
> On 10/23/2015 08:32 AM, Dmitry V. Levin wrote:
> > Hi,
> > 
> > On Thu, Oct 22, 2015 at 05:27:24PM +0200, Denys Vlasenko wrote:
> >> Hello Dmitry,
> >>
> >> Two years ago, at the end of 2013, strace source had 29 *.c files
> >> in its main source directory (meaning: this does not count any tests).
> >> linux/* contained 148 files in total (not only *.c files).
> >>
> >> Today, there are 120 *.c files in main source directory,
> >> and 419 files in linux/*
> > 
> > Ideally, each parser should have been in a separate file from the
> > beginning, that would make maintenance easier.
> > I'm working in this direction.
> 
> It's easier in what way?

You must be joking.  Smaller translation unit has less context to care
about.  For example, there used to be unwanted side effects when headers
included for one parser affected other parsers.  In case of recently
introduced mpers parsers like SYS_FUNC(msgctl), it's necessary because
such files are compiled several times.

> >> I have hard time understanding code now. Register reading logic
> >> for x86 is now in what, a dozen different files?
> >>
> >> Take a look at x86_64/get_scno.c - what's there is not even a
> >> complete function definition, it's a fragment of C code to be
> >> #included somewhere else. It's hard to see what's being done
> >> where, let alone spot possible bugs or inefficiencies.
> > 
> > Ideally, *.c file in the main directory should have been without arch
> > ifdefs at all, arch specific parts should have been placed in arch
> > subdirs from the beginning, that would make maintenance easier.
> 
> This does not answer my question regarding readability.
> 
> Let's get back to linux/x86_64/get_scno.c
> Here's the entire beginning of the file, I'm skipping nothing:
> 
> 
> #ifndef __X32_SYSCALL_BIT
> # define __X32_SYSCALL_BIT      0x40000000
> #endif
> 
> unsigned int currpers;
> 
> #if 1
> /* ... */
> if (x86_io.iov_len == sizeof(i386_regs)) {
>         scno = i386_regs.orig_eax;
>         currpers = 1;
> } else {
> 
> 
> So. What is "scno"? Is is a global variable? A local one?
> A function parameter?
> 
> Is it a "long"? Or "unsigned long long"?
> 
> To learn answers to these questions, I need to go
> to *another file* and look there!
> 
> Usually, in C language people don't have to do that
> to find out such things.

Do you mean that an arch_get_scno function called by generic get_scno
would be easier to read than current #include "get_scno.c"?
Yes, I think it would.

> This is not easier. This is more obscure. IMHO, of course.
> You probably have logical reasons why I'm not right in my opinion.
> Can you voice these reasons?

Mixing numerous arch specific ifdefs with generic code is much harder to
read and more error prone that letting compiler choose the right code
automatically by including arch specific implementation from a separate
arch specific file.

> > Now that arch specific parts of get_scno are moved to arch subdirs,
> > I can maintain this code and explain others how it works.
> > 
> >> Another example:
> >>
> >> commit 9b2f674adbd5c44fe892b31cf95703eeceb21c40
> >> Author: Dmitry V. Levin <ldv at altlinux.org>
> >> Date:   Sat Dec 6 03:53:16 2014 +0000
> >>
> >>     file.c: move chdir parser to a separate file
> >>
> >> I have a question: Why it's better to have it in a separate file?
> >> I mean, the
> >>
> >> #include "defs.h"
> >>
> >> SYS_FUNC(chdir)
> >> {
> >>         printpath(tcp, tcp->u_arg[0]);
> >>
> >>         return RVAL_DECODED;
> >> }
> >>
> >> needs a separate file why?
> > 
> > file.c was a huge mess, I cleaned it up as much as I could by moving all
> > non-stat parsers to separate files.  The remaining part of file.c is still
> > a mess, but now it's in a more maintainable state.
> 
> Sorry. You are saying that it's cleaner this way, but you don't explain
> *why* it's cleaner this way. I don't see any particular benefits
> when a larger *.c file gets split into three dozen tiny ones.

Sorry, I think it's obvious.  The larger the source file, the more
unrelated context is there and more chances of unwanted side effects.  At
the same time, I see no benefits from unrelated parsers being crammed into
large source files.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20151024/6e24fc76/attachment.bin>