From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 128718 invoked by alias); 3 Mar 2016 15:25:08 -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 128708 invoked by uid 89); 3 Mar 2016 15:25:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=Lines, Hx-languages-length:3084, H*RU:209.85.218.68, Hx-spam-relays-external:209.85.218.68 X-HELO: mail-oi0-f68.google.com Received: from mail-oi0-f68.google.com (HELO mail-oi0-f68.google.com) (209.85.218.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 03 Mar 2016 15:25:07 +0000 Received: by mail-oi0-f68.google.com with SMTP id m82so1357251oif.1 for ; Thu, 03 Mar 2016 07:25:06 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=JXp08eDnpK5JvKS6MBdoWxwXynLndEQ5glMq2V1GteA=; b=aZjphMGeZqYk44AZ21xKHgW19VNCW/V0/KDcFMJfh5tppDISR8Fr+5lKIWH20uLYY8 A6uV1lY27OlWfSjh3CujDaxijAMV0uhDHDkzSukrXsDImLCkr8ijXpNB1aTEj4vhBH9b Scqx2Qy9B0RrImtLA9jwC75/B8aV4tMIfKvnKbTfRD71d/ttbS4TBJdCbUpvYa0zs7fN WEYBl1Tn13Rj24cInoUpkGsMdGypnp1QNTFI0AeGOL843pj49+nkbwS5K8hony7ahtFn 09ikAHBzGrwNutoCBDbfDA/iWMc1U4snM3P2YJChsPLUTPHfw4FtuPyaYdanTwsBeE2R Rjmw== X-Gm-Message-State: AD7BkJJN+f2eUKvnmLoEDJRVr4bMxqxh8wqsBd6aDQoMtjGU6A4qMEP2+5rn258W313RIAicf5r5UP8KzxxSOQ== X-Received: by 10.202.200.206 with SMTP id y197mr1855767oif.92.1457018705236; Thu, 03 Mar 2016 07:25:05 -0800 (PST) MIME-Version: 1.0 Received: by 10.182.29.226 with HTTP; Thu, 3 Mar 2016 07:24:45 -0800 (PST) In-Reply-To: <1457018483-26829-1-git-send-email-dmalcolm@redhat.com> References: <1457018483-26829-1-git-send-email-dmalcolm@redhat.com> From: Patrick Palka Date: Thu, 03 Mar 2016 15:25:00 -0000 Message-ID: Subject: Re: [PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0) To: David Malcolm Cc: GCC Patches Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2016-03/txt/msg00266.txt.bz2 On Thu, Mar 3, 2016 at 10:21 AM, David Malcolm wrote: > PR c/68187 covers two cases involving poor indentation where > the indentation is arguably not misleading, but for which > -Wmisleading-indentation emits a warning. > > The two cases appear to be different in nature; one in comment #0 > and the other in comment #1. Richi marked the bug as a whole as > a P1 regression; it's not clear to me if he meant one or both of > these cases, so the following two patches fix both. > > The rest of this post addresses the case in comment #0 of the PR; > the followup post addresses the other case, in comment #1 of the PR. > > Building glibc (a9224562cbe9cfb0bd8d9e637a06141141f9e6e3) on x86_64 > led to this diagnostic from -Wmisleading-indentation: > > ../stdlib/strtol_l.c: In function '____strtoul_l_internal': > ../stdlib/strtol_l.c:356:9: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation] > cnt < thousands_len; }) > ^ > ../stdlib/strtol_l.c:353:9: note: ...this 'for' clause, but it is not > && ({ for (cnt = 0; cnt < thousands_len; ++cnt) > ^ > > The code is question looks like this: > > 348 for (c = *end; c != L_('\0'); c = *++end) > 349 if (((STRING_TYPE) c < L_('0') || (STRING_TYPE) c > L_('9')) > 350 # ifdef USE_WIDE_CHAR > 351 && (wchar_t) c != thousands > 352 # else > 353 && ({ for (cnt = 0; cnt < thousands_len; ++cnt) > 354 if (thousands[cnt] != end[cnt]) > 355 break; > 356 cnt < thousands_len; }) > 357 # endif > 358 && (!ISALPHA (c) > 359 || (int) (TOUPPER (c) - L_('A') + 10) >= base)) > 360 break; > > Lines 354 and 355 are poorly indented, leading to the warning from > -Wmisleading-indentation at line 356. > > The wording of the warning is clearly wrong: line 356 isn't indented as if > guarded by line 353, it's more that lines 354 and 355 *aren't* indented. > > What's happening is that should_warn_for_misleading_indentation has a > heuristic for handling "} else", such as: > > if (p) > foo (1); > } else // GUARD > foo (2); // BODY > foo (3); // NEXT > > and this heuristic uses the first non-whitespace character in the line > containing GUARD as the column of interest: the "}" character. > > In this case we have: > > 353 && ({ for (cnt = 0; cnt < thousands_len; ++cnt) // GUARD > 354 if (thousands[cnt] != end[cnt]) // BODY > 355 break; > 356 cnt < thousands_len; }) // NEXT > > and so it uses the column of the "&&", and treats it as if it were > indented thus: > > 353 for (cnt = 0; cnt < thousands_len; ++cnt) // GUARD > 354 if (thousands[cnt] != end[cnt]) // BODY > 355 break; > 356 cnt < thousands_len; }) // NEXT > > and thus issues a warning. > > As far as I can tell the heuristic in question only makes sense for > "else" clauses, so the following patch updates it to only use the > special column when handling "else" clauses, eliminating the > overzealous warning. Wouldn't this change have the undesirable effect of no longer warning about: if (p) foo (1); } else if (q) foo (2); foo (3);