On 25 Jun 2024, at 12:22, Ales Musil wrote:
> 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. Guess it’s not clear to me what you are going to use to lint only the diff. I was assuming the tool you were using will tell you why you are not compliant (maybe hinting to the configuration parameter), which you could then grep on. But I have not looked at tooling around this for quite a while. Last time I used this was just for vscode to correct my coding on the fly (but that was a couple of years ago). //Eelco _______________________________________________ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
