On Tue, Jun 25, 2024 at 12:14 PM Eelco Chaudron <echau...@redhat.com> wrote:
> > > 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). > Wouldn't that be very hard to detect? If we get diff of things that should be changed I'm not sure how to detect if it's something that we should ignore. > > > >>>>>> [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> > > -- 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
