Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Jan Beulich <JBeulich@xxxxxxxx>]

On 02.08.2019 13:44, Viktor Mitin wrote:
> On Fri, Aug 2, 2019 at 12:23 PM Juergen Gross <jgross@xxxxxxxx> wrote:
> 
>>>> A snipplet from commit 57f8b13c724023c78fa15a80452d1de3e51a1418:
>>>>
>>>> @@ -4096,14 +4097,12 @@ retry_transaction:
>>>>             goto out;
>>>>
>>>>         if (target == NULL) {
>>>> -        libxl__xs_printf(gc, t, target_path, "%"PRIu32,
>>>> -                         (uint32_t) info.current_memkb);
>>>> -        *target_memkb = (uint32_t) info.current_memkb;
>>>> +        libxl__xs_printf(gc, t, target_path, "%"PRIu64,
>>>> info.current_memkb);
>>>> +        *target_memkb = info.current_memkb;
>>>>         }
>>>>         if (staticmax == NULL) {
>>>> -        libxl__xs_printf(gc, t, max_path, "%"PRIu32,
>>>> -                         (uint32_t) info.max_memkb);
>>>> -        *max_memkb = (uint32_t) info.max_memkb;
>>>> +        libxl__xs_printf(gc, t, max_path, "%"PRIu64, info.max_memkb);
>>>> +        *max_memkb = info.max_memkb;
>>>>         }
>>>>
>>>>         rc = 0;
>>>>
>>>
>>> I've build gnu diffutils latest release 3.7 and checked the code and
>>> the issue. It seems this feature (-p) doesn't work properly and has
>>> many more bugs than just the issue with labels. See the example below,
>>> the change has been made in the function a1(), however, in the diff
>>> another function is shown a().
>>
>> This case is completely fine, as the context of the diff is starting
>> before the definition of a() (and just after a1()). This is important in
>> case you only add a new function for example.
>>
> 
> Juergen, the diff returns wrong name of the function silently. I don't
> think it is 'completely fine', I would rather call it a bug, or
> non-documented limitation, but definitely not a feature. As I wrote
> previously, I played with -p a little and observed more similar issues
> with it. The reason is that the next diff code (see below) tries to
> match function names using simple regexp for a line, assuming that
> line with function name cannot start with a 'non-alpha' characters. So
> even adding one more space before a line with function name breaks it.

And there's not supposed to be any blank ahead of a function's
heading.

> diffutils-3.7/src/diff.c:462
>      case 'p':
>        show_c_function = true;
>        add_regexp (&function_regexp_list, "^[[:alpha:]$_]");
> 
> It is probably could be improved by adding 'no : symbol at the end of
> the line' logic to this regexp. It will resolve the issue with labels,
> but will add one more bug (or limitation) about using ":" in such
> lines.
> 
> The issue is more general here. The diff tool should not parse any
> programming languages, it should just compare the strings.

"The strings" being which ones? The limitations (if you want to call
them such) of "diff -p" are, I think, well understood. And there's
no "parsing" here at all - the regexp simply checks the first
character. The time when you wanted to make it skip labels would be
where it would become more like "parsing".

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel