Re: [Libguestfs] [PATCH 0/2] added filesystem_walk0 low level API
On Wednesday 30 March 2016 13:45:15 NoxDaFox wrote:
Thanks for the comments, just few notes and questions.
Just remember to keep the list in the loop.
2016-03-30 12:24 GMT+03:00 Pino Toscano <ptoscano(a)redhat.com>:
> Hi,
>
> while reviewing the new series of tsk-related changes, I'm noticing
> common mistakes in all of them: I'll list them below, not
> copy/paste-ing them every review.
>
> (a) please check the coding style, in particular
> - the lack of space between a function name and its following opening
> parenthesis; e.g. do_something(param) -> do_something (param)
>
I was complying with what said here:
http://libguestfs.org/guestfs-hacking.1.html
It refers to K&R style which suggests functions defined as
do_something(param).
I guess the documentation at the link could mention this detail.
Indeed it is something that should be changed, since the rest of the
code definitely has spaces between functions and opening parenthesis.
- the style of functions, e.g.:
> | int do_something (...)
> needs to be
> | int
> | do_something (...)
>
Same as above.
What I mentioned is the style used all around the code.
- style of function prototypes (everything in a single line)
>
I'm confused here: should they be all on a single line or should they be
split as in the note above?
Prototypes need a single line, while the actual function definitions
indented as above.
In general, a golden rule that applies whenever patching any software
is "follow the existing style", in case there is a single one (or at
least one followed in the majority of the codebase).
> (b) please avoid if statements with side effects, as it makes
code
> reading and refactoring harder
>
Will re-factor the logic.
> (c) please put explanations and message in the messages of the actual
> commits, instead of in the cover letters sent to the mailing list:
> otherwise, browsing the git history in the future will not give many
> clues about the newly added APIs
>
I tried to stick to the behaviour I saw in the mailing list. Will fix this
as well.
Usually cover letters provide a general overview of what a series of
patches does, while the actual commits still provide explanations of
what each does.
> (d) please wrap descriptions for actions at 80 columns max.
>
Going to fix this as well.
--
Pino Toscano
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)