Patchwork [1/5] CODING_STYLE: add preprocessor rules

login
register
mail settings
Submitter Blue Swirl
Date Aug. 12, 2010, 5:49 p.m.
Message ID <AANLkTikqSLRUw9WYVQoA9tbaaa9tgNjYmrRV-EEv7DbW@mail.gmail.com>
Download mbox | patch
Permalink /patch/61637/
State New
Headers show

Comments

Blue Swirl - Aug. 12, 2010, 5:49 p.m.
Add preprocessor rules from libvirt HACKING.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 CODING_STYLE |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)
malc - Aug. 12, 2010, 6:47 p.m.
On Thu, 12 Aug 2010, Blue Swirl wrote:

> Add preprocessor rules from libvirt HACKING.
> 
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  CODING_STYLE |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/CODING_STYLE b/CODING_STYLE
> index 92036f3..c4c09ab 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -79,3 +79,16 @@ and clarity it comes on a line by itself:
>  Rationale: a consistent (except for functions...) bracing style reduces
>  ambiguity and avoids needless churn when lines are added or removed.
>  Furthermore, it is the QEMU coding style.
> +
> +5. Preprocessor
> +
> +For variadic macros, stick with C99 syntax:
> +
> +#define vshPrint(_ctl, ...)   fprintf(stdout, __VA_ARGS__)

_ctl is not used inside the macro expansion, furthermore i'd avoid
using leading underscore even for macro arguments.

> +
> +Use parenthesis when checking if a macro is defined, and use
> +indentation to track nesting:
> +
> +#if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
> +# define fallocate(a,ignored,b,c) posix_fallocate(a,b,c)
> +#endif
>
Blue Swirl - Aug. 13, 2010, 4:56 p.m.
On Thu, Aug 12, 2010 at 6:47 PM, malc <av1474@comtv.ru> wrote:
> On Thu, 12 Aug 2010, Blue Swirl wrote:
>
>> Add preprocessor rules from libvirt HACKING.
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  CODING_STYLE |   13 +++++++++++++
>>  1 files changed, 13 insertions(+), 0 deletions(-)
>>
>> diff --git a/CODING_STYLE b/CODING_STYLE
>> index 92036f3..c4c09ab 100644
>> --- a/CODING_STYLE
>> +++ b/CODING_STYLE
>> @@ -79,3 +79,16 @@ and clarity it comes on a line by itself:
>>  Rationale: a consistent (except for functions...) bracing style reduces
>>  ambiguity and avoids needless churn when lines are added or removed.
>>  Furthermore, it is the QEMU coding style.
>> +
>> +5. Preprocessor
>> +
>> +For variadic macros, stick with C99 syntax:
>> +
>> +#define vshPrint(_ctl, ...)   fprintf(stdout, __VA_ARGS__)
>
> _ctl is not used inside the macro expansion, furthermore i'd avoid
> using leading underscore even for macro arguments.

Right. Moreover the macro name is too libvirt'ish.

>> +
>> +Use parenthesis when checking if a macro is defined, and use
>> +indentation to track nesting:
>> +
>> +#if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
>> +# define fallocate(a,ignored,b,c) posix_fallocate(a,b,c)
>> +#endif

This one is new, current code doesn't use indentation. Maybe it's
better to drop it.
Andreas Färber - Aug. 14, 2010, 2:27 p.m.
Am 13.08.2010 um 18:56 schrieb Blue Swirl:

>>> +Use parenthesis when checking if a macro is defined, and use
>>> +indentation to track nesting:
>>> +
>>> +#if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
>>> +# define fallocate(a,ignored,b,c) posix_fallocate(a,b,c)
>>> +#endif
>
> This one is new, current code doesn't use indentation. Maybe it's
> better to drop it.

Yes, please. While indentation is nice to read, it is patch-unfriendly.

Consider:

#ifdef SOMETHINGNEW
yay
#else // previous stuff follows

#if defined(OLDONE)
#  define one
#endif
foo
bar
#if defined(OLDTWO)
#  define two
#endif

#endif

Here, adding four lines would require to reindent six lines otherwise  
not changed, making merging and review harder.

Andreas
Blue Swirl - Aug. 15, 2010, 7:46 a.m.
On Sat, Aug 14, 2010 at 2:27 PM, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 13.08.2010 um 18:56 schrieb Blue Swirl:
>
>>>> +Use parenthesis when checking if a macro is defined, and use
>>>> +indentation to track nesting:
>>>> +
>>>> +#if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
>>>> +# define fallocate(a,ignored,b,c) posix_fallocate(a,b,c)
>>>> +#endif
>>
>> This one is new, current code doesn't use indentation. Maybe it's
>> better to drop it.
>
> Yes, please. While indentation is nice to read, it is patch-unfriendly.
>
> Consider:
>
> #ifdef SOMETHINGNEW
> yay
> #else // previous stuff follows
>
> #if defined(OLDONE)
> #  define one
> #endif
> foo
> bar
> #if defined(OLDTWO)
> #  define two
> #endif
>
> #endif
>
> Here, adding four lines would require to reindent six lines otherwise not
> changed, making merging and review harder.

I don't quite follow the example, but is the situation different from
adding a C 'if' statement and adjusting the indentation?

However, I'm not proposing this anymore.
Andreas Färber - Aug. 15, 2010, 7:59 a.m.
Am 15.08.2010 um 09:46 schrieb Blue Swirl:

> On Sat, Aug 14, 2010 at 2:27 PM, Andreas Färber <andreas.faerber@web.de 
> > wrote:
>> While indentation is nice to read, it is patch-unfriendly.
>>
>> Consider:
>>
>> #ifdef SOMETHINGNEW
>> yay
>> #else // previous stuff follows
>>
>> #if defined(OLDONE)
>> #  define one
>> #endif
>> foo
>> bar
>> #if defined(OLDTWO)
>> #  define two
>> #endif
>>
>> #endif
>>
>> Here, adding four lines would require to reindent six lines  
>> otherwise not
>> changed, making merging and review harder.
>
> I don't quite follow the example,

The middle part would need to become:

#  if defined(OLDONE)
#    define one
#  endif
foo
bar
#  if defined(OLDTWO)
#    define two
#  endif

if we applied the rule. Purely syntactic example.

> but is the situation different from
> adding a C 'if' statement and adjusting the indentation?

No, you're right, it isn't. Just that we previously applied such  
changes for Windows or Solaris and did not need to reindent.

Cheers,
Andreas

Patch

diff --git a/CODING_STYLE b/CODING_STYLE
index 92036f3..c4c09ab 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -79,3 +79,16 @@  and clarity it comes on a line by itself:
 Rationale: a consistent (except for functions...) bracing style reduces
 ambiguity and avoids needless churn when lines are added or removed.
 Furthermore, it is the QEMU coding style.
+
+5. Preprocessor
+
+For variadic macros, stick with C99 syntax:
+
+#define vshPrint(_ctl, ...)   fprintf(stdout, __VA_ARGS__)
+
+Use parenthesis when checking if a macro is defined, and use
+indentation to track nesting:
+
+#if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
+# define fallocate(a,ignored,b,c) posix_fallocate(a,b,c)
+#endif