Message ID | 20180518182907.xhpjfk2jj2f3t65e@adacore.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 34707 invoked by alias); 18 May 2018 18:29:13 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 34673 invoked by uid 89); 18 May 2018 18:29:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_NUMSUBJECT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=ari, sk:memory-, sk:memory, Standards X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 18 May 2018 18:29:11 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 9C9B7117DA8; Fri, 18 May 2018 14:29:09 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id DZh5fg+i9v+C; Fri, 18 May 2018 14:29:09 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 7416D117D90; Fri, 18 May 2018 14:29:09 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id A71E78304E; Fri, 18 May 2018 11:29:07 -0700 (PDT) Date: Fri, 18 May 2018 11:29:07 -0700 From: Joel Brobecker <brobecker@adacore.com> To: Simon Marchi <simark@simark.ca> Cc: gdb-patches@sourceware.org Subject: Re: New ARI warning Fri May 18 01:56:48 UTC 2018 Message-ID: <20180518182907.xhpjfk2jj2f3t65e@adacore.com> References: <20180518015648.GA106395@sourceware.org> <cf20cf67-976e-9233-4f7e-f8ff7b94e816@simark.ca> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="cib3lvix5qd4gnkf" Content-Disposition: inline In-Reply-To: <cf20cf67-976e-9233-4f7e-f8ff7b94e816@simark.ca> User-Agent: NeoMutt/20170113 (1.7.2) |
Commit Message
Joel Brobecker
May 18, 2018, 6:29 p.m. UTC
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.
Comments
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
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