Possible bug: two_comp_mod has no export_bytes() call

[jafingerhut]

I don't know the implementation details well enough to know whether this is a bug -- was just looking at the source code for two_comp_mod and other operators in behavioral-model code, and noted that all of the methods implementing operators in class Data that return void end with a call to export_bytes(), except two_comp_mod:

https://github.com/p4lang/behavioral-model/blob/master/include/bm/bm_sim/data.h#L317

Here are add and sub, for comparison:

https://github.com/p4lang/behavioral-model/blob/master/include/bm/bm_sim/data.h#L220

I don't have a test case exhibiting wrong behavior here -- just a suspicion that two_comp_mod may have been left behind when others were updated at some point in the past, and it may not be exercised in any test cases since it is a relatively rarely used operator (I think only when signed values occur in P4 source code, which is uncommon).

[antoninbas]

Could it be because the operation actually doesn't change the bit pattern, in which case there would be no need to call export_bytes?
If this was my intention, I should probably have written a comment explaining this when I wrote the code...

[jafingerhut]

I have since found that two_comp_mod is exercised by a P4_16 test case in p4lang/p4c named arith5-bmv2.p4, and also a couple of P4_14 programs contain the operator in their bmv2 JSON files. The arith5-bmv2.stf test cases look reasonable to me, so maybe no bug here.

When you say "the operation doesn't change the bit pattern", do you mean the output bit pattern is always equal to the input bit pattern? That would have a much shorter implementation in lines of code, wouldn't it? (e.g. "value = src;")

[antoninbas]

Discard my original comment, it's incorrect. I thought the operator was doing something else entirely.
A call to export_bytes is indeed needed. The p4c test cases cannot catch this bug because two_comp_mod is never applied directly on the Field. Instead the result of the expression is stored in a local Data variable, whose value is then assigned to the long-lived Field object. The assignment operator (Data::set) calls export_bytes which means that the bit representation is updated correctly.