On 3/18/25 12:01, Dumitru Ceara wrote: > On 3/18/25 11:48 AM, Ilya Maximets wrote: >> On 3/18/25 11:32, Dumitru Ceara wrote: >>> On 3/18/25 11:26 AM, Aleksandr Smirnov (K2 Cloud) wrote: >>>> Hi guys, >>>> >>> >>> Hi Alexander, >>> >>>> Could I kindly ask you not to bring to C-language comments any unnatural >>>> meaning? >>>> I mean let's keep comments as just comments. You can move 'cksum = NNN' >>>> away from >>>> comment to other entity like static variable with cost of few bytes or >>>> even #define that >>>> will cost you nothing. >>>> >>> >>> That's a fair ask, a #define is OK from my perspective. >> >> Hmm. >> >> The point of having this checksum in the comment that belongs to the >> version definition is to create a strong relation between them. >> If we create a separate define, we'll need to add another comment to >> it explaining that if this is changed than the version change should >> be considered. And we already have this for the pipeline stages and >> the actions and it doesn't really work. Closer we have it, more >> chances for it to be noticed in review. >> > > I wouldn't be against something like: > > --- a/lib/ovn-util.c > +++ b/lib/ovn-util.c > @@ -843,11 +843,15 @@ ip_address_and_port_from_lb_key(const char *key, char > **ip_address, > return true; > } > > -/* Increment this for any logical flow changes, if an existing OVN action is > - * modified or a stage is added to a logical pipeline. > +/* Increment OVN_INTERNAL_MINOR_VER for any logical flow changes, if an > + * existing OVN action is modified or a stage is added to a logical pipeline. > * > * This value is also used to handle some backward compatibility during > - * upgrading. It should never decrease or rewind. */ > + * upgrading. It should never decrease or rewind. > + * > + * NOTE: if OVN_NORTHD_PIPELINE_CSUM is updated make sure to double check > + * whether an update of OVN_INTERNAL_MINOR_VER is required. */ > +#define OVN_NORTHD_PIPELINE_CSUM xyzt > #define OVN_INTERNAL_MINOR_VER 6 > > But I have no strong preference I guess. > >> What exactly the point of not having the checksum in the comment? >> We sum some text and then grep it out as text. At no point this >> checksum is a structured data or part of the language construct. >> AFAICT, we will lose on clarity, but I'm not sure what we'll gain. >> > > I think I understand a bit where Alexander is coming from; it happens to > me every now and then to ignore comments (and not read them even though > I should) because the code itself is self-explanatory. A change of a > #define value would trigger me as reviewer to be more thorough, probably.
I'm not convinced, but the diff above seems OK (even though it's much harder to read, IMO, with all the capital letters in the comment), so I will not insist on having it in the comment. > >> FWIW, comments are used for code generation in other places, e.g. >> OpenFlow fields. >> >> Best regards, Ilya Maximets. >> >>> >>>> Thank you, >>>> >>>> Alexander >>>> >>> >>> Regards, >>> Dumitru >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
