| Message ID | 20180228150616.20390-1-phil@nwl.cc |
|---|---|
| State | Accepted |
| Delegated to: | Florian Westphal |
| Headers | show |
| Series | [nft] Review switch statements for unmarked fall through cases | expand |
Phil Sutter <phil@nwl.cc> wrote: > While revisiting all of them, clear a few oddities as well: > > - There's no point in marking empty fall through cases: They are easy to > spot and a common concept when using switch(). NACK, sorry. There a source-code checkers that flag this (they have heuristics to detect such comments). I agree with the rest and would apply the patch w.o. the first hunk, no need to resend. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Florian, On Wed, Feb 28, 2018 at 04:33:05PM +0100, Florian Westphal wrote: > Phil Sutter <phil@nwl.cc> wrote: > > While revisiting all of them, clear a few oddities as well: > > > > - There's no point in marking empty fall through cases: They are easy to > > spot and a common concept when using switch(). > > NACK, sorry. There a source-code checkers that flag this > (they have heuristics to detect such comments). > > I agree with the rest and would apply the patch w.o. the first hunk, > no need to resend. Initial motivation for this review came from a covscan report, so this patch is actually to make those static code checkers happy, not the other way around. :) Regarding empty fall through (which seems to be the reason for your NACK): There was but a single fall through comment for an empty case in the whole code, and there are literally hundreds of them. Covscan didn't complain about those, hence why I think even from that perspective the single one is pointless. Cheers, Phil -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Phil Sutter <phil@nwl.cc> wrote: > Regarding empty fall through (which seems to be the reason for your > NACK): There was but a single fall through comment for an empty case in > the whole code, and there are literally hundreds of them. Covscan didn't > complain about those, hence why I think even from that perspective the > single one is pointless. ok, fair enough, applied, thanks Phil. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/ct.c b/src/ct.c index d5347974bd0d5..bdf50d7a46d6b 100644 --- a/src/ct.c +++ b/src/ct.c @@ -289,7 +289,7 @@ static void ct_print(enum nft_ct_keys key, int8_t dir, uint8_t nfproto, } switch (key) { - case NFT_CT_SRC: /* fallthrough */ + case NFT_CT_SRC: case NFT_CT_DST: desc = proto_find_upper(&proto_inet, nfproto); if (desc) diff --git a/src/evaluate.c b/src/evaluate.c index 6be3bf031f587..54fd6b61dbbdf 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -2712,6 +2712,7 @@ static int stmt_evaluate_objref_map(struct eval_ctx *ctx, struct stmt *stmt) map->mappings->set->flags |= map->mappings->set->init->set_flags; + /* fall through */ case EXPR_SYMBOL: if (expr_evaluate(ctx, &map->mappings) < 0) return -1; diff --git a/src/hash.c b/src/hash.c index 9cd3c8cfa173d..3355cadd1df2f 100644 --- a/src/hash.c +++ b/src/hash.c @@ -20,7 +20,7 @@ static void hash_expr_print(const struct expr *expr, struct output_ctx *octx) switch (expr->hash.type) { case NFT_HASH_SYM: nft_print(octx, "symhash"); - break; + break; case NFT_HASH_JENKINS: default: nft_print(octx, "jhash "); diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c index d06b227eb9924..bec5d82fda9c7 100644 --- a/src/netlink_delinearize.c +++ b/src/netlink_delinearize.c @@ -1494,6 +1494,7 @@ static void ct_meta_common_postprocess(struct rule_pp_ctx *ctx, case OP_NEQ: if (right->ops->type != EXPR_SET && right->ops->type != EXPR_SET_REF) break; + /* fall through */ case OP_LOOKUP: expr_set_type(right, left->dtype, left->byteorder); break; diff --git a/src/rule.c b/src/rule.c index 5b7219e82eaaa..a53908f670c1d 100644 --- a/src/rule.c +++ b/src/rule.c @@ -1341,7 +1341,7 @@ static void obj_print_data(const struct obj *obj, } } break; - case NFT_OBJECT_CT_HELPER: { + case NFT_OBJECT_CT_HELPER: nft_print(octx, "ct helper %s {\n", obj->handle.obj); nft_print(octx, "\t\ttype \"%s\" protocol ", obj->ct_helper.name); @@ -1349,7 +1349,6 @@ static void obj_print_data(const struct obj *obj, nft_print(octx, "\t\tl3proto %s", family2str(obj->ct_helper.l3proto)); break; - } case NFT_OBJECT_LIMIT: { bool inv = obj->limit.flags & NFT_LIMIT_F_INV; const char *data_unit; @@ -1661,11 +1660,13 @@ static int do_command_reset(struct netlink_ctx *ctx, struct cmd *cmd) switch (cmd->obj) { case CMD_OBJ_COUNTERS: dump = true; + /* fall through */ case CMD_OBJ_COUNTER: type = NFT_OBJECT_COUNTER; break; case CMD_OBJ_QUOTAS: dump = true; + /* fall through */ case CMD_OBJ_QUOTA: type = NFT_OBJECT_QUOTA; break;
While revisiting all of them, clear a few oddities as well: - There's no point in marking empty fall through cases: They are easy to spot and a common concept when using switch(). - Fix indenting of break statement in one occasion. - Drop needless braces around one case which doesn't declare variables. Signed-off-by: Phil Sutter <phil@nwl.cc> --- src/ct.c | 2 +- src/evaluate.c | 1 + src/hash.c | 2 +- src/netlink_delinearize.c | 1 + src/rule.c | 5 +++-- 5 files changed, 7 insertions(+), 4 deletions(-)