From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 127904 invoked by alias); 3 Mar 2016 15:56:46 -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 127882 invoked by uid 89); 3 Mar 2016 15:56:44 -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:5967, thousands, interest, 350 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; Thu, 03 Mar 2016 15:56:42 +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 709F464D0B; Thu, 3 Mar 2016 15:56:41 +0000 (UTC) Received: from vpn-231-38.phx2.redhat.com (vpn-231-38.phx2.redhat.com [10.3.231.38]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u23Fuetd004861; Thu, 3 Mar 2016 10:56:41 -0500 Message-ID: <1457020600.1637.29.camel@redhat.com> Subject: Re: [PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0) From: David Malcolm To: Patrick Palka Cc: GCC Patches Date: Thu, 03 Mar 2016 15:56:00 -0000 In-Reply-To: References: <1457018483-26829-1-git-send-email-dmalcolm@redhat.com> Content-Type: multipart/mixed; boundary="=-2vbvuZzq+RxugZfeX2NX" Mime-Version: 1.0 X-IsSubscribed: yes X-SW-Source: 2016-03/txt/msg00271.txt.bz2 --=-2vbvuZzq+RxugZfeX2NX Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Content-length: 4844 On Thu, 2016-03-03 at 10:24 -0500, Patrick Palka wrote: > 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); No, because the rejection based on indentation is done relative to guard_line_first_nws, rather than guard_vis_column (I tried doing it via the latter in one version of the patch, and that broke some of the existing cases, so I didn't make that change). See the attached test file (which doesn't have dg-directives yet); the example you give is test1_d, with an open-brace added to the "if (p)". Trunk emits warnings for: * test1_c * test1_d * test1_e * test1_f (two warnings, one for the "if", one for the "else") * test1_g * test2_c * test2_d * test2_e With the patches, it emits warnings for: * test1_c * test1_d * test1_e * test1_f (just one warnings, for the "if") * test1_g * test2_c * test2_d * test2_e so the only change is the removal of the erroneous double warning for the "else" in test1_f. I can add dg-directives and add the attachment to Wmisleading -indentation.c as part of the patch (or keep it as a separate new test file, the former is getting large). --=-2vbvuZzq+RxugZfeX2NX Content-Disposition: attachment; filename="Wmisleading-indentation-4.c" Content-Type: text/x-csrc; name="Wmisleading-indentation-4.c"; charset="UTF-8" Content-Transfer-Encoding: base64 Content-length: 2481 LyogUFIgYy82ODE4Ny4gICovCi8qIHsgZGctb3B0aW9ucyAiLVdtaXNsZWFk aW5nLWluZGVudGF0aW9uIiB9ICovCi8qIHsgZGctZG8gY29tcGlsZSB9ICov CgpleHRlcm4gaW50IGZvbyAoaW50KTsKZXh0ZXJuIGludCBiYXIgKGludCwg aW50KTsKZXh0ZXJuIGludCBmbGFnQTsKZXh0ZXJuIGludCBmbGFnQjsKZXh0 ZXJuIGludCBmbGFnQzsKZXh0ZXJuIGludCBmbGFnRDsKCi8qIEJlZm9yZSB0 aGUgIn0iLiAgKi8KCnZvaWQKdGVzdDFfYSAodm9pZCkKewogIGlmIChmbGFn QSkgewogICAgZm9vICgxKTsKICB9IGVsc2UgaWYgKGZsYWdCKQogZm9vICgy KTsKIGZvbyAoMyk7Cn0KCi8qIEFsaWduZWQgd2l0aCB0aGUgIn0iLiAgKi8K CnZvaWQKdGVzdDFfYiAodm9pZCkKewogIGlmIChmbGFnQSkgewogICAgZm9v ICgxKTsKICB9IGVsc2UgaWYgKGZsYWdCKQogIGZvbyAoMik7CiAgZm9vICgz KTsKfQoKLyogSW5kZW50ZWQgYmV0d2VlbiB0aGUgIn0iIGFuZCB0aGUgImVs c2UiLiAgKi8KCnZvaWQKdGVzdDFfYyAodm9pZCkKewogIGlmIChmbGFnQSkg ewogICAgZm9vICgxKTsKICB9IGVsc2UgaWYgKGZsYWdCKQogICBmb28gKDIp OwogICBmb28gKDMpOwp9CgovKiBBbGlnbmVkIHdpdGggdGhlICJlbHNlIi4g ICovCgp2b2lkCnRlc3QxX2QgKHZvaWQpCnsKICBpZiAoZmxhZ0EpIHsKICAg IGZvbyAoMSk7CiAgfSBlbHNlIGlmIChmbGFnQikKICAgIGZvbyAoMik7CiAg ICBmb28gKDMpOwp9CgovKiBJbmRlbnRlZCBiZXR3ZWVuIHRoZSAiZWxzZSIg YW5kIHRoZSAiaWYiLiAgKi8KCnZvaWQKdGVzdDFfZSAodm9pZCkKewogIGlm IChmbGFnQSkgewogICAgZm9vICgxKTsKICB9IGVsc2UgaWYgKGZsYWdCKQog ICAgICBmb28gKDIpOwogICAgICBmb28gKDMpOwp9CgovKiBBbGlnbmVkIHdp dGggdGhlICJpZiIuICAqLwoKdm9pZAp0ZXN0MV9mICh2b2lkKQp7CiAgaWYg KGZsYWdBKSB7CiAgICBmb28gKDEpOwogIH0gZWxzZSBpZiAoZmxhZ0IpCiAg ICAgICAgIGZvbyAoMik7CiAgICAgICAgIGZvbyAoMyk7Cn0KCi8qIEluZGVu dGVkIG1vcmUgdGhhbiB0aGUgImlmIi4gICovCgp2b2lkCnRlc3QxX2cgKHZv aWQpCnsKICBpZiAoZmxhZ0EpIHsKICAgIGZvbyAoMSk7CiAgfSBlbHNlIGlm IChmbGFnQikKICAgICAgICAgICAgZm9vICgyKTsKICAgICAgICAgICAgZm9v ICgzKTsKfQoKLyogQWdhaW4sIGJ1dCB3aXRob3V0IHRoZSAybmQgImlmIi4g ICovCgovKiBCZWZvcmUgdGhlICJ9Ii4gICovCgp2b2lkCnRlc3QyX2EgKHZv aWQpCnsKICBpZiAoZmxhZ0EpIHsKICAgIGZvbyAoMSk7CiAgfSBlbHNlCiBm b28gKDIpOwogZm9vICgzKTsKfQoKLyogQWxpZ25lZCB3aXRoIHRoZSAifSIu ICAqLwoKdm9pZAp0ZXN0Ml9iICh2b2lkKQp7CiAgaWYgKGZsYWdBKSB7CiAg ICBmb28gKDEpOwogIH0gZWxzZQogIGZvbyAoMik7CiAgZm9vICgzKTsKfQoK LyogSW5kZW50ZWQgYmV0d2VlbiB0aGUgIn0iIGFuZCB0aGUgImVsc2UiLiAg Ki8KCnZvaWQKdGVzdDJfYyAodm9pZCkKewogIGlmIChmbGFnQSkgewogICAg Zm9vICgxKTsKICB9IGVsc2UKICAgZm9vICgyKTsKICAgZm9vICgzKTsKfQoK LyogQWxpZ25lZCB3aXRoIHRoZSAiZWxzZSIuICAqLwoKdm9pZAp0ZXN0Ml9k ICh2b2lkKQp7CiAgaWYgKGZsYWdBKSB7CiAgICBmb28gKDEpOwogIH0gZWxz ZQogICAgZm9vICgyKTsKICAgIGZvbyAoMyk7Cn0KCi8qIEluZGVudGVkIG1v cmUgdGhhbiB0aGUgImVsc2UiLiAgKi8KCnZvaWQKdGVzdDJfZSAodm9pZCkK ewogIGlmIChmbGFnQSkgewogICAgZm9vICgxKTsKICB9IGVsc2UKICAgICAg ICBmb28gKDIpOwogICAgICAgIGZvbyAoMyk7Cn0K --=-2vbvuZzq+RxugZfeX2NX--