New ARI warning Fri May 18 01:56:48 UTC 2018

  Hi Simon,

> >> gdb/unittests/format_pieces-selftests.c:51: code: %ll: Do not use printf(%ll), instead use printf(%s,phex()) to dump a 'long long' value
> > gdb/unittests/format_pieces-selftests.c:51:  check ("Hello %d%llx%%d",
> >> gdb/unittests/format_pieces-selftests.c:56: code: %ll: Do not use printf(%ll), instead use printf(%s,phex()) to dump a 'long long' value
> > gdb/unittests/format_pieces-selftests.c:56:      format_piece ("%llx", long_long_arg),
> > 
> 
> These are false positives.

You can tag them as OK with a /* ARI: ... */ comment.

But I suspect we just want to exclude files in gdb/unittests instead?
Here is a patch that does that. Tested by checking the change in
output before and after:

    1101,1120d1100
    < ./unittests/array-view-selftests.c
    < ./unittests/common-utils-selftests.c
    < ./unittests/environ-selftests.c
    < ./unittests/function-view-selftests.c
    < ./unittests/lookup_name_info-selftests.c
    < ./unittests/memory-map-selftests.c
    < ./unittests/memrange-selftests.c
    < ./unittests/observable-selftests.c
    < ./unittests/offset-type-selftests.c
    < ./unittests/optional-selftests.c
    < ./unittests/ptid-selftests.c
    < ./unittests/rsp-low-selftests.c
    < ./unittests/scoped_fd-selftests.c
    < ./unittests/scoped_mmap-selftests.c
    < ./unittests/scoped_restore-selftests.c
    < ./unittests/string_view-selftests.c
    < ./unittests/tracepoint-selftests.c
    < ./unittests/unpack-selftests.c
    < ./unittests/utils-selftests.c
    < ./unittests/xml-utils-selftests.c

gdb/ChangeLog:

        * contrib/ari/gdb_find.sh: Exclude the unittest directory.
  

On 2018-05-18 14:29, Joel Brobecker wrote:
> Hi Simon,
> 
>> >> gdb/unittests/format_pieces-selftests.c:51: code: %ll: Do not use printf(%ll), instead use printf(%s,phex()) to dump a 'long long' value
>> > gdb/unittests/format_pieces-selftests.c:51:  check ("Hello %d%llx%%d",
>> >> gdb/unittests/format_pieces-selftests.c:56: code: %ll: Do not use printf(%ll), instead use printf(%s,phex()) to dump a 'long long' value
>> > gdb/unittests/format_pieces-selftests.c:56:      format_piece ("%llx", long_long_arg),
>> >
>> 
>> These are false positives.
> 
> You can tag them as OK with a /* ARI: ... */ comment.
> 
> But I suspect we just want to exclude files in gdb/unittests instead?
> Here is a patch that does that. Tested by checking the change in
> output before and after:
> 
>     1101,1120d1100
>     < ./unittests/array-view-selftests.c
>     < ./unittests/common-utils-selftests.c
>     < ./unittests/environ-selftests.c
>     < ./unittests/function-view-selftests.c
>     < ./unittests/lookup_name_info-selftests.c
>     < ./unittests/memory-map-selftests.c
>     < ./unittests/memrange-selftests.c
>     < ./unittests/observable-selftests.c
>     < ./unittests/offset-type-selftests.c
>     < ./unittests/optional-selftests.c
>     < ./unittests/ptid-selftests.c
>     < ./unittests/rsp-low-selftests.c
>     < ./unittests/scoped_fd-selftests.c
>     < ./unittests/scoped_mmap-selftests.c
>     < ./unittests/scoped_restore-selftests.c
>     < ./unittests/string_view-selftests.c
>     < ./unittests/tracepoint-selftests.c
>     < ./unittests/unpack-selftests.c
>     < ./unittests/utils-selftests.c
>     < ./unittests/xml-utils-selftests.c
> 
> gdb/ChangeLog:
> 
>         * contrib/ari/gdb_find.sh: Exclude the unittest directory.

I don't really mind, maybe some rules related to formatting would still 
be appropriate for unittests/.  Is is possible to exclude unittests/* 
instead of listing all the files?  We'll surely add new files in there, 
and don't want to have to update that script every time.

Simon
  

> > You can tag them as OK with a /* ARI: ... */ comment.
> > 
> > But I suspect we just want to exclude files in gdb/unittests instead?
> > Here is a patch that does that. Tested by checking the change in
> > output before and after:
> > 
> >     1101,1120d1100
> >     < ./unittests/array-view-selftests.c
> >     < ./unittests/common-utils-selftests.c
> >     < ./unittests/environ-selftests.c
> >     < ./unittests/function-view-selftests.c
> >     < ./unittests/lookup_name_info-selftests.c
> >     < ./unittests/memory-map-selftests.c
> >     < ./unittests/memrange-selftests.c
> >     < ./unittests/observable-selftests.c
> >     < ./unittests/offset-type-selftests.c
> >     < ./unittests/optional-selftests.c
> >     < ./unittests/ptid-selftests.c
> >     < ./unittests/rsp-low-selftests.c
> >     < ./unittests/scoped_fd-selftests.c
> >     < ./unittests/scoped_mmap-selftests.c
> >     < ./unittests/scoped_restore-selftests.c
> >     < ./unittests/string_view-selftests.c
> >     < ./unittests/tracepoint-selftests.c
> >     < ./unittests/unpack-selftests.c
> >     < ./unittests/utils-selftests.c
> >     < ./unittests/xml-utils-selftests.c
> > 
> > gdb/ChangeLog:
> > 
> >         * contrib/ari/gdb_find.sh: Exclude the unittest directory.
> 
> I don't really mind, maybe some rules related to formatting would still be
> appropriate for unittests/.

Right, which is why I formulated this as a question. I don't really
know what style we want there. But since it was easy to patch the
script, I thought I'd send that right away, to show one of the options.
But I'd be OK with deciding that unittests/ should follow the GDB
coding standards.

> Is is possible to exclude unittests/* instead of listing all the
> files?  We'll surely add new files in there, and don't want to have to
> update that script every time.

This is exactly what the patch I sent does. The above was just
the diff of output between before and after patch (sorry for
the cryptic message).

     find "$@" \
         -name testsuite -prune -o \
    +    -name unittests -prune -o \
         -name gdbserver -prune -o \
         -name gdbtk -prune -o \
         -name gnulib -prune -o \
  

On 2018-05-18 15:03, Joel Brobecker wrote:
> Right, which is why I formulated this as a question. I don't really
> know what style we want there. But since it was easy to patch the
> script, I thought I'd send that right away, to show one of the options.
> But I'd be OK with deciding that unittests/ should follow the GDB
> coding standards.

I think we can just add the /* ARI: ... */ comments, I'll try it later.  
I don't see any reason why it would be harder in general to follow our 
code conventions in unit tests than anywhere else.  This case is just a 
bit of a special one.

>> Is is possible to exclude unittests/* instead of listing all the
>> files?  We'll surely add new files in there, and don't want to have to
>> update that script every time.
> 
> This is exactly what the patch I sent does. The above was just
> the diff of output between before and after patch (sorry for
> the cryptic message).
> 
>      find "$@" \
>          -name testsuite -prune -o \
>     +    -name unittests -prune -o \
>          -name gdbserver -prune -o \
>          -name gdbtk -prune -o \
>          -name gnulib -prune -o \

Ahh ok, I missed the patch in attachment.  I indeed thought that the 
output you pasted was the actual patch in a cryptic format (like diff's 
default format).

Simon