[sane-devel] Coding style/Code quality (was Re: [janitorial] Text file style convention "enforcement")

[Olaf Meeuwissen]

Hi Aaron,

Thanks for the feedback.  Details inlined below.

Aaron Muir Hamilton writes:

> Olaf Meeuwissen <paddy-hack at member.fsf.org> writes:
>
>> [...]
>> # I wondered what to do with leading empty lines but decided not to
>> # touch these (for now).  They might serve some sort of "aesthetic"
>> # purpose.  I also noticed a note on "follow the GNU coding standards"
>> # in doc/backend-writing.txt (and shied away from that, for now?).  As
>> # for the tab vs spaces mess?  Let's leave that for later.

> There's also GNU indent, which defaults to GNU style these days, and
> should be installed on most of our workstations (except perhaps on
> OpenBSD or FreeBSD, but possibly there as well).

It's not the tool that's the problem.  There's lots of code "beautifier"
programs to choose from (too many maybe).  The problem is that backend
maintainers have their own preferences and they may not be all the same.

I can ride roughshod over everybody, convert all code files to the GNU
coding standards in a jiffy with automated checking to boot and whack
complainers over the head with a quote from doc/backend-writing.txt but
that's not gonna make me (m)any friends ;-)

> It might be interesting to ask people to run it on a file when they make
> a considerable change and there are no known outstanding patches against
> the file. Or even if there are outstanding patches which conflict with
> style repairs, one can format the patch branch and it should apply.

Backend maintainers are free to reformat/sanitize the code for *their*
backend whenever they like, IMHO.  When co-maintaining, please get some
kind of agreement between the co-maintainers first.  Whatever style you
choose, pick something that's easily checked(/fixed) by a common tool.

# People may want to add something to their pre-commit hook.  For the
# builder(s), if it's in Debian stable that's good enough.

> Honestly my greatest concern is that the genesys_gl*.c per-controller
> backends will drift further apart. The 800-series ones are still very
> similar, but the 124[+] backend has some of the function definitions
> shifted around in order, but otherwise nearly identical in function.
> I'm trying to get my hands on a scanner matching each genesys controller
> revision so that I can regression test anything I do in that realm.
>
> Those files are largely identical, the functional differences are
> limited to a few differing or new function definitions, and the magic
> values used in given registers (though the register offsets remain
> largely the same).

Sounds like a case of source code cloning.  That doesn't scale.  Been
there, done that.  It's hell.  Refactor the common functions into a
single file, parameterize the differences and you should be able to get
rid off a fair bit of code.

I haven't looked at the code much but did notice similarities while
fixing all the compiler warnings.  Maybe we should run a copy-n-paste
detector over the code base.

> The more the files drift apart, the harder it'll be to a) realize that
> fixes should be ported between them and b) that they are substantially
> similar, neigh on identical files.

Don't port fixes between them.  Use a single copy of the code for all of
them.  Just put each duplicated function in its own separate .c file and
include that directly (or declare the function extern), whatever works.

You can do this in a separate branch so others can give feedback.  I'll
be happy to take a peek and give feedback on the general direction.  I
won't be able to check all the little details.

> Anyway, if all goes well I should be receiving four more genesys-based
> scanners, including a couple not mentioned on the website. I'll also be
> getting a whole stack of calibration targets so I can tell if something
> has gone subtly wrong for a given mode on a given backend. :- )

But if you have lots of hardware to test with ... those details should
be taken care of ;-)

> Boy, that was a bit off-topic, do go on.

By all means, do get off-topic, or rather off-on-a-tangent, every once
in a while.  The list could do with some more developer talk ;-)

Hope this helps,
--
Olaf Meeuwissen, LPIC-2            FSF Associate Member since 2004-01-27
 GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13  F43E B8A4 A88A F84A 2DD9
 Support Free Software                        https://my.fsf.org/donate
 Join the Free Software Foundation              https://my.fsf.org/join