On 6/25/24 08:54, Ales Musil wrote: > On Tue, Jun 25, 2024 at 8:48 AM Eelco Chaudron <echau...@redhat.com> wrote: > >> >> >> On 24 Jun 2024, at 17:52, Ales Musil via discuss wrote: >> >>> Hi, >>>
Hi Ales, Thanks for bringing this up! >>> I would like to propose a universal coding style using clang-format [0]. >>> I've put together the config file that is matching the current coding >> style >>> as closely as possible [1]. I've tried it on some random pieces of code >>> that we have in the OVN codebase and there are some instances where it >>> doesn't much 1:1 (down below). The idea would be to add this into CI and >>> run this only on diff so we don't have to modify old code. Let me know >> what >>> you think and if it would be acceptable. In the end we can have a style >> and >>> let some pieces of code diverge on that. >> >> I like the idea as it makes the maintainer’s live easier, as some might >> slip through, and we can always ignore a specific warning if some other >> style is used in the whole file. If it’s a directory you can override the >> parent config file. >> I agree, it will still be up to the maintainer to "waive" some of the warnings if the suggested change would make the code look unnatural. But, in my opinion, that should be fine. > >>> Before: >>> ctl_error(ctx, "Same routing policy already existed on the " >>> "logical router %s.", ctx->argv[1]); >>> >>> After: >>> >>> ctl_error(ctx, >>> "Same routing policy already existed on the logical " >>> "router %s.", >>> ctx->argv[1]); >> >> This might be something to get use too, or ignore (in GitHub), as almost >> all code will continue filling up the next line(s). >> > > Yeah it's a bit strange, however still readable, it really depends on the > length of the string in this case. If the string fits into one line it's > capable of having multiple arguments next to each other. > Same here, it's not great but also not that terrible. I think it's probably acceptable. > >> >>> Before: >>> return (out_port != VIGP_CONTROL_PATH >>> ? alpheus_output_port(dp, skb, out_port) >>> : alpheus_output_control(dp, skb, fwd_save_skb(skb), >>> VIGR_ACTION)); >>> >>> After: >>> return ( >>> out_port != VIGP_CONTROL_PATH >>> ? alpheus_output_port(dp, skb, out_port) >>> : alpheus_output_control(dp, skb, fwd_save_skb(skb), >> VIGR_ACTION)); >> >> I assume this is a none problem, as you do not need the outer parenthesis >> on return. Or is this for general match with parenthesis? >> > > This is specific (from what I can tell) to return statements. Without the > parenthesis it's way better: > > return out_port != VIGP_CONTROL_PATH > ? alpheus_output_port(dp, skb, out_port) > : alpheus_output_control(dp, skb, fwd_save_skb(skb), > VIGR_ACTION); > > This one I don't really like but hopefully we won't have a lot of these situations (multi-line ternary). We should probably add clear documentation (maybe we should improve the contribution guide in general) and mention that warnings generated for multi-line ternary can be ignored and the documented coding style should be used instead. Regards, Dumitru > >> >>> Thanks, >>> Ales >>> >>> >>> [0] https://gist.github.com/almusil/d9fc05c9e9ab6cef9448e123b49c351e >>> [1] >> https://docs.ovn.org/en/stable/internals/contributing/coding-style.html >>> >>> -- >>> >>> Ales Musil >>> >>> Senior Software Engineer - OVN Core >>> >>> Red Hat EMEA <https://www.redhat.com> >>> >>> amu...@redhat.com >>> <https://red.ht/sig> >> >>> _______________________________________________ >>> discuss mailing list >>> disc...@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-discuss >> >> > _______________________________________________ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
