From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5558 invoked by alias); 4 Mar 2016 07:15:34 -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 5493 invoked by uid 89); 4 Mar 2016 07:15:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:4655, thousands, picked, interest X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 04 Mar 2016 07:15:25 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 123788F4EB for ; Fri, 4 Mar 2016 07:15:24 +0000 (UTC) Received: from slagheap.utah.redhat.com ([10.3.113.3]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u247FNLa002610; Fri, 4 Mar 2016 02:15:23 -0500 Subject: Re: [PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0) To: David Malcolm , gcc-patches@gcc.gnu.org References: <1457018483-26829-1-git-send-email-dmalcolm@redhat.com> From: Jeff Law Message-ID: <56D9360B.7030600@redhat.com> Date: Fri, 04 Mar 2016 07:15:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1457018483-26829-1-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2016-03/txt/msg00312.txt.bz2 On 03/03/2016 08: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. > > Doing so led to a nonsensical warning for > libstdc++-v3/src/c++11/random.cc:random_device::_M_init: > > random.cc: In member function ‘void std::random_device::_M_init(const string&)’: > random.cc:102:10: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] > else if (token != "/dev/urandom" && token != "/dev/random") > ^~ > random.cc:107:5: note: ...this statement, but the latter is indented as if it does > _M_file = static_cast(std::fopen(fname, "rb")); > ^~~~~~~ > > so the patch addresses this by tweaking the heuristic that rejects > aligned BODY and NEXT so that it doesn't require them to be aligned with > the first non-whitespace of the GUARD, simply that they not be indented > relative to it. > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu in > combination with the following patch; standalone bootstrap®rtest > is in progress. > > OK for trunk if the latter is successful? > > gcc/c-family/ChangeLog: > PR c/68187 > * c-indentation.c (should_warn_for_misleading_indentation): When > suppressing warnings about cases where the guard and body are on > the same column, only use the first non-whitespace column in place > of the guard token column when dealing with "else" clauses. > When rejecting aligned BODY and NEXT, loosen the requirement > from equality with the first non-whitespace of guard to simply > that they not be indented relative to it. > > gcc/testsuite/ChangeLog: > PR c/68187 > * c-c++-common/Wmisleading-indentation.c (fn_40_a): New test > function. > (fn_40_b): Likewise. > (fn_41_a): Likewise. > (fn_41_b): Likewise. OK. FWIW, I think all of c-indentation would fall under the diagnostics maintainership you picked up recently. jeff