On Tue, Jun 25, 2024 at 12:41 PM Eelco Chaudron <echau...@redhat.com> wrote:
> > > 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 > > So there is a git-clang-format tool that can report diff what is supposed to be applied, or it can do the fixing itself. The clang-format has something called dry run, which reports only warning/errors but nothing specific. All are classified as "code should be clang-formatted [-Wclang-format-violations]". I don't think we can ignore specific ones. Thanks, Ales -- 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
