Hi,
On 07/05/2020 09:58, Jan Beulich wrote:
On 07.05.2020 10:35, Julien Grall wrote:
On 07/05/2020 08:21, Jan Beulich wrote:
On 06.05.2020 18:44, Paul Durrant wrote:
+#define DOMAIN_SAVE_BEGIN(_x, _c, _v, _len) \
+ domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_len))
In new code I'd like to ask for no leading underscores on macro
parameters as well as no unnecessary parenthesization of macro
parameters (e.g. when they simply get passed on to another macro
or function without being part of a larger expression).
Personally I think it is generally good practice to parenthesize
but I can drop if you prefer.
To be honest - it's more than just "prefer": Excess parentheses
hamper readability. There shouldn't be too few, and since macros
already require quite a lot of them, imo we should strive to
have exactly as many as are needed.
While I understand that too many parentheses may make the code
worse, in the case of the macros, adding them for each argument
is a good practice. This pretty simple to follow and avoid the
mistake to forget to protect an argument correctly.
So I would let the contributor decides whether he wants to
protect all the macro arguments or just as a need basis.
This is a possible alternative position to take, accepting the
worse readability. But then this would please need applying in
full (as far as it's possible - operands to # or ## of course
can't be parenthesized):
#define DOMAIN_SAVE_BEGIN(x, c, v, len) \
domain_save_begin((c), DOMAIN_SAVE_CODE((x)), #x, (v), (len))
which might look a little odd even to you?
One pair of parenthesis looks wasteful but it doesn't bother that much.
As to readability - I'm sure you realize that from time to time
one needs to look at preprocessor output, where parentheses used
like this plus parentheses then also applied inside the invoked
macro add to one another. All in all I don't really buy the
"avoid the mistake to forget to protect an argument correctly"
argument to outweigh the downsides. We're doing this work not on
an occasional basis, but as a job. There should be a minimum
level of care everyone applies.
I am sure you heard about "human error" in the past. Even if you do
something all day along, you will make a mistake one day. By requesting
a contributor to limit the number of parenthesis, then you increase the
chance he/she will screw up one day.
You might think reviewers will catch such error, but XSA-316 proved that
it is possible to miss it. I was the author of the patch and you were
one the reviewer. As you can see even an experienced contributor and
reviewer can make mistake... we are all human after all ;).
I don't ask to be over-proctective, but we should not lay trap to other
contributors for them to fall into accidentally. In this case, Paul
thinks the parentheses will help him. So I would not impose him to
remove them and possibly make a mistake.
Cheers,
--
Julien Grall
