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

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

[GDB Administrator]

644a645,646
> 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),

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

[Simon Marchi]

On 2018-05-17 09:56 PM, GDB Administrator wrote:
> 644a645,646
>> 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.

Simon

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

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.

-- 
Joel

[-- Attachment #2: 0001-gdb-Do-not-apply-ARI-checks-to-unittests-files.patch --]
[-- Type: text/x-diff, Size: 911 bytes --]

From 68f15bf62a21633b8bf5c1f4c68722a2a1f0c7c8 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Fri, 18 May 2018 11:25:52 -0700
Subject: [PATCH] (gdb) Do not apply ARI checks to unittests files

This directory contains files that are for testing purposes only,
and so don't really have to confirm to the GDB Coding Standards.

gdb/ChangeLog:

        * contrib/ari/gdb_find.sh: Exclude the unittest directory.
---
 gdb/contrib/ari/gdb_find.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/contrib/ari/gdb_find.sh b/gdb/contrib/ari/gdb_find.sh
index 304761832a..0be71635bd 100644
--- a/gdb/contrib/ari/gdb_find.sh
+++ b/gdb/contrib/ari/gdb_find.sh
@@ -31,6 +31,7 @@ LC_ALL=C ; export LC_ALL
 
 find "$@" \
     -name testsuite -prune -o \
+    -name unittests -prune -o \
     -name gdbserver -prune -o \
     -name gdbtk -prune -o \
     -name gnulib -prune -o \
-- 
2.11.0

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

[Simon Marchi]

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

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

[Joel Brobecker]

> > 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 \

-- 
Joel

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

[Simon Marchi]

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

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

[Simon Marchi]

On 2018-05-18 03:14 PM, Simon Marchi wrote:
> 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.

Here's what I pushed:

From 1d143c36eedc0f0b124e6aa6fb3b98b1e6ff74b0 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Fri, 18 May 2018 15:47:56 -0400
Subject: [PATCH] format_pieces-selftests.c: Silence ARI warnings

Silence this:

unittests/format_pieces-selftests.c:51: warning: code: Do not use printf("%ll"), instead use printf("%s",phex()) to dump a `long long' value
unittests/format_pieces-selftests.c:56: warning: code: Do not use printf("%ll"), instead use printf("%s",phex()) to dump a `long long' value

gdb/ChangeLog:

	* unittests/format_pieces-selftests.c (test_format_specifier):
	Add ARI comments.
---
 gdb/ChangeLog                           | 5 +++++
 gdb/unittests/format_pieces-selftests.c | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d4d00f8..57edb23 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-05-18  Simon Marchi  <simon.marchi@ericsson.com>
+
+	* unittests/format_pieces-selftests.c (test_format_specifier):
+	Add ARI comments.
+
 2018-05-18  Tom Tromey  <tom@tromey.com>

 	* c-typeprint.c (maybe_print_hole): New function.
diff --git a/gdb/unittests/format_pieces-selftests.c b/gdb/unittests/format_pieces-selftests.c
index 6d11a9c..8f63247 100644
--- a/gdb/unittests/format_pieces-selftests.c
+++ b/gdb/unittests/format_pieces-selftests.c
@@ -48,12 +48,12 @@ test_escape_sequences ()
 static void
 test_format_specifier ()
 {
-  check ("Hello %d%llx%%d",
+  check ("Hello %d%llx%%d", /* ARI: %ll */
     {
       format_piece ("Hello ", literal_piece),
       format_piece ("%d", int_arg),
       format_piece ("", literal_piece),
-      format_piece ("%llx", long_long_arg),
+      format_piece ("%llx", long_long_arg), /* ARI: %ll */
       format_piece ("%%d", literal_piece),
     });
 }
-- 
2.7.4