On Wed, Jul 2, 2014 at 1:11 AM, David Laight <david.lai...@aculab.com> wrote: > From: Alexei Starovoitov > ... >> >> +#define _(OP) ({ int ret = OP; if (ret < 0) return ret; }) > ... >> >> + _(get_map_info(env, map_id, &map)); >> > >> > Nit: such macros should be removed, please. >> >> It may surely look unconventional, but alternative is to replace >> every usage of _ macro with: >> err = >> if (err) >> return err; >> >> and since this macro is used 38 times, it will add ~120 unnecessary >> lines that will only make code much harder to follow. >> I tried not using macro and results were not pleasing. > > The problem is that they are hidden control flow. > As such they make flow analysis harder for the casual reader.
In the abstract context macros with gotos and returns are bad, but in this case extra verbosity is the bigger evil. Consider this piece of code: #define _(OP) ({ int ret = OP; if (ret < 0) return ret; }) if (opcode == BPF_END || opcode == BPF_NEG) { if (BPF_SRC(insn->code) != BPF_X) return -EINVAL; /* check src operand */ _(check_reg_arg(regs, insn->dst_reg, 1)); /* check dest operand */ _(check_reg_arg(regs, insn->dst_reg, 0)); } else if (opcode == BPF_MOV) { if (BPF_SRC(insn->code) == BPF_X) /* check src operand */ _(check_reg_arg(regs, insn->src_reg, 1)); /* check dest operand */ _(check_reg_arg(regs, insn->dst_reg, 0)); where casual reader can easily see what the purpose of the code and what it's doing. Now rewrite it without '_' macro: if (opcode == BPF_END || opcode == BPF_NEG) { if (BPF_SRC(insn->code) != BPF_X) return -EINVAL; /* check src operand */ err = check_reg_arg(regs, insn->dst_reg, 1); if (err) return err; /* check dest operand */ err = check_reg_arg(regs, insn->dst_reg, 0); if (err) return err; } else if (opcode == BPF_MOV) { if (BPF_SRC(insn->code) == BPF_X) { /* check src operand */ err = check_reg_arg(regs, insn->src_reg, 1); if (err) return err; } /* check dest operand */ err = check_reg_arg(regs, insn->dst_reg, 0); if (err) return err; see how your eyes are now picking up endless control flow of if conditions and returns, instead of focusing on the code itself. It's much easier to understand the semantics when if (err) is out of the way. Note that replacing _ with real name will ruin the reading experience, since CAPITAL letters of the macro will be screaming: "look at me", instead of letting reviewer focus on the code. I believe that this usage of _ as a macro specifically as defined, would be a great addition to kernel coding style in general. I don't want to see _ to be redefined differently. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/