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. > //Eelco > > >>>> [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
