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.
> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
