From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 77522 invoked by alias); 2 Feb 2017 18:00:50 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 77510 invoked by uid 89); 2 Feb 2017 18:00:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=hassle, warrant, documenting, subconscious X-HELO: mail-qt0-f193.google.com Received: from mail-qt0-f193.google.com (HELO mail-qt0-f193.google.com) (209.85.216.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 02 Feb 2017 18:00:48 +0000 Received: by mail-qt0-f193.google.com with SMTP id s58so5553019qtc.2 for ; Thu, 02 Feb 2017 10:00:48 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=PF7h1/avx4HeFT8wgJsvx9E7yGS5SgxwymAutoOwgcs=; b=Amdzr0VU1XSEJt+iXZMglJVKP+cg6uvjKTPCx5/pcde0+4GNsEaq0dqVLeJNgp5tMl 1LZOWfpOfJehFk9CfvtpSY+S7o64FGmCqXfkKOK1FesLuGlU+NHr1j2IN3I/pCukhyPC 7RaIkP/wTIWPmHQVXjBtUBUvCJFYOUZw1iclcwgMmD/EIYye04x4WeVbqQ9Z+qDmknOI 8ktH69mm8l74Mc5YRPw3t32GOg5jLDqnAkpfChFJpu2ULw2ntkUu1JssHQOHCwkb0xNt rlZsyA1zxaZSft+3cKkJgv+VmgeTWRB6HX5GIzpyv2tj4+rzlnCHOd8eweR6eQZ1bqGQ UsNQ== X-Gm-Message-State: AIkVDXLD3jq49Axr1BEZdHV4DzkoK9+jpGxp028DWIHOtYny5mhUDHvqDR+9QPm19HizGQ== X-Received: by 10.200.50.18 with SMTP id x18mr9922668qta.58.1486058446466; Thu, 02 Feb 2017 10:00:46 -0800 (PST) Received: from [192.168.0.3] (97-118-107-34.hlrn.qwest.net. [97.118.107.34]) by smtp.gmail.com with ESMTPSA id z123sm19526114qkd.45.2017.02.02.10.00.45 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 02 Feb 2017 10:00:46 -0800 (PST) Subject: Re: [PATCH] relax -Wformat-overflow for precision ranges (PR 79275) To: Jeff Law , Gcc Patch List References: <236621e2-1755-be9f-0b44-0bc9c8356d85@gmail.com> From: Martin Sebor Message-ID: Date: Thu, 02 Feb 2017 18:00:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-02/txt/msg00195.txt.bz2 On 02/02/2017 10:26 AM, Jeff Law wrote: > On 01/30/2017 02:28 PM, Martin Sebor wrote: >> Bug 79275 - -Wformat-overflow false positive exceeding INT_MAX in >> glibc sysdeps/posix/tempname.c points out a false positive found >> during a Glibc build and caused by the checker using the upper >> bound of a range of precisions in string directives with string >> arguments of non-constant length. The attached patch relaxes >> the checker to use the lower bound instead when appropriate. >> >> Martin >> >> gcc-79275.diff >> >> >> PR middle-end/79275 - -Wformat-overflow false positive exceeding >> INT_MAX in glibc sysdeps/posix/tempname.c >> >> gcc/testsuite/ChangeLog: >> >> PR middle-end/79275 >> * gcc.dg/tree-ssa/builtin-sprintf-warn-11.c: New test. >> * gcc.dg/tree-ssa/pr79275.c: New test. >> >> gcc/ChangeLog: >> >> PR middle-end/79275 >> * gimple-ssa-sprintf.c (get_string_length): Set lower bound to zero. >> (format_string): Tighten up the range of output for non-constant >> strings and correct the expected range for wide non-constant strings. > Couple more nits. > > First, I expect the patch won't apply as-is with the operand order > fixes. There'll be trivial changes you'll need to make for that. I had to back out those changes to avoid the massive conflict when applying this patch and before retesting it. Then I reapplied those changes by hand. > > Along the same lines, this patch would introduce a new operand order nit > here: > > >> + } >> + else if (0 <= dir.prec[1]) >> + { In light of the hassle this has caused (above, plus with the big patch earlier) I can't help but question the value in making these changes. Isn't the code just as easy to read this way as the other? else if (dir.prec[1] >= 0) Wouldn't it be if the 0 were spelled ZERO? I never even thought about it until you started pointing it out, but my own personal (subconscious) bias is clear from how I write the code. I'm also not the only one. FWIW, the example above may not be one of them but there are cases when the constant on the left actually makes the code much easier to read than the other way, such as: gcc/read-rtl-function.c: if (0 == strncmp (desc, "orig:", 5)) or cp/decl.c: if (1 == simple_cst_equal (TREE_PURPOSE (t1) It seems to me that we should be able to write these expressions the way that's natural to us and at the same time be able to comfortably read them both ways. As always, I fully support consistency and following a coding style where it matters. I just don't think this does. > > Please consider documenting how we handle strings with unknown lengths. > > > I don't think those warrant waiting for another review round. Fix, > bootstrap, test and install. Will do. Thanks Martin