On 25 Jun 2024, at 11:53, Ales Musil wrote:
> On Tue, Jun 25, 2024 at 11:20 AM Eelco Chaudron <echau...@redhat.com> wrote: > >> >> >> On 25 Jun 2024, at 10:11, Dumitru Ceara wrote: >> >>> 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. >> >> I have not used clang’s linter for a while, but can you tell it not to >> warn/ignore specific styles? If not, maybe we can have the GitHub wrapper >> ignore specific style warnings. >> > > > You can use annotation on specific pieces of code via simple comment e.g. > "/*clang-format off */". We could use those to skip the warnings if needed. I do not think we should add this, as the code would quickly become a mess. I was suggesting that when you run this on the diff during the GitHub action, to add a wrapper that will not report certain violations, i.e. the ones we do not care about (do not like). >>>>>> [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 >>>>> >>>>> >>>> >> >> > > -- > > 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
