On 14/01/2020 17:27, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 16/20] tools/libxl: Simplify callback handling
> in libxl-save-helper"):
>> The {save,restore}_callback helpers can have their scope reduced vastly,
> This part is OK with me although it would have been nicer to review if
> the the move and the rename were separate patches. I don't know why
> it is valuable.
>
>> and helper_setcallbacks_{save,restore}() doesn't need to use a
>> ternary operator to write 0 (meaning NULL) into an already zeroed
>> structure.
> Is this unrelated ? I think so.
This change is specifically to make the generated C easier to follow,
because I had to debug it yet again.
>> my $c_cb = "cbs->$name";
>> $f_more_sr->(" if ($c_cb) cbflags |= $c_v;\n", $enumcallbacks);
>> - $f_more_sr->(" $c_cb = (cbflags & $c_v) ? ${encode}_${name} :
>> 0;\n",
>> + $f_more_sr->(" if (cbflags & $c_v) $c_cb = ${encode}_${name};\n",
>> $setcallbacks);
> It is a long time since I edited this code but I think your reasoning
> is "cbs is already zero on entry because it is static; therefore
> cbs->$name must be null, so there is no need to write 0 into it in the
> else case".
Correct.
>
> However, the line you are touching is preceded by "if ($c_cb)" which
> only makes sense if the variable might be non-null.
>
> So something is not right here.
This is all perl to me, but the two adjacent-looking lines of C don't
end up adjacent in the generated code.
The first line ends up in
libxl__srm_callout_enumcallbacks_{save,restore}() (in libxl.so), while
the second line ends up in helper_setcallbacks_{save,restore}() (in
libxl-save-helper).
~Andrew
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
