Am 15.11.19 um 21:37 schrieb Markus Elfring:
>> This eliminates duplication in the semantic patch, which is good.
>
> Thanks that you think in such a direction.
>
>
>> It messes up the indentation of n in some of the cases in 921d49be86 ("use
>> COPY_ARRAY for copying arrays", 2019-06-15), though. Hmm, but that can
>> be cured by duplicating the comma:
>
> I have picked up further improvement possibilities for this SmPL script.
> Would you like to integrate any of these changes?
Not sure, could you please elaborate on the benefits of each proposed
change?
> @@
> expression dst, src, n, E;
> @@
> memcpy(dst, src, sizeof(
> + *(
> E
> - [...]
> + )
> ) * n
> )
That's longer and looks more complicated to me than what we currently have:
@@
expression dst, src, n, E;
@@
memcpy(dst, src, n * sizeof(
- E[...]
+ *(E)
))
Avoiding to duplicate E doesn't seem to be worth it. I can see that
indenting the sizeof parameter and parentheses could improve readability,
though.
> @@
> type T;
> T *ptr;
> T[] arr;
> expression E, n;
> @@
> memcpy(
> ( ptr, E, sizeof(
> - *(ptr)
> + T
> ) * n
> | arr, E, sizeof(
> - *(arr)
> + T
> ) * n
> | E, ptr, sizeof(
> - *(ptr)
> + T
> ) * n
> | E, arr, sizeof(
> - *(arr)
> + T
> ) * n
> )
> )
This still fails to regenerate two of the changes from 921d49be86 (use
COPY_ARRAY for copying arrays, 2019-06-15), at least with for me (and
Coccinelle 1.0.4).
> @@
> type T;
> T* dst_ptr, src_ptr;
> T[] dst_arr, src_arr;
> expression n, x;
> @@
> -memcpy
> +COPY_ARRAY
> (
> ( dst_ptr
> | dst_arr
> )
> ,
> ( src_ptr
> | src_arr
> )
> - , (n) * \( sizeof(T) \| sizeof(*(x)) \)
> + , n
> )
That x could be anything -- it's not tied to the element size of source
or destination. Such a transformation might change the meaning of the
code, as COPY_ARRAY will use the element size of the destination behind
the scenes. So that doesn't look safe to me.
> @@
> type T;
> T* dst, src, ptr;
> expression n;
> @@
> (
> -memmove
> +MOVE_ARRAY
> (dst, src
> - , (n) * \( sizeof(* \( dst \| src \) ) \| sizeof(T) \)
> + , n
> )
> |
> -ptr = xmalloc((n) * \( sizeof(*ptr) \| sizeof(T) \))
> +ALLOC_ARRAY(ptr, n)
> );
memmove/MOVE_ARRAY and xmalloc/ALLOC_ARRAY are quite different; why
would we want to jam transformations for them into the same rule like
this? The only overlap seems to be n. Handling memmove/MOVE_ARRAY and
memcpy/COPY_ARRAY together would make more sense, as they take the same
kinds of parameters.
I didn't know that disjunctions can be specified inline using \(, \|
and \), though. Rules can be much more compact that way. Mixing
languages like that can also be quite confusing. Syntax highlighting
could help; https://github.com/ahf/cocci-syntax at least doesn't
show those any different from regular code, though.
> Now I observe that the placement of space characters can be a coding style
> concern at four places for adjusted lines by the generated patch.
> Would you like to clarify remaining issues for pretty-printing
> in such use cases?
Ideally, generated code should adhere to Documentation/CodingGuidelines,
so that it can be accepted without requiring hand-editing.
René
