From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 49756 invoked by alias); 11 Jul 2017 17:28:51 -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 48795 invoked by uid 89); 11 Jul 2017 17:28:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-6.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy=associates, intentional, symptoms, manifest X-HELO: mail-qk0-f196.google.com Received: from mail-qk0-f196.google.com (HELO mail-qk0-f196.google.com) (209.85.220.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 11 Jul 2017 17:28:48 +0000 Received: by mail-qk0-f196.google.com with SMTP id 16so1740684qkg.2 for ; Tue, 11 Jul 2017 10:28:48 -0700 (PDT) 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=7ImGYZXD8lTAVwYYBpkEFRYTlnUZ+MANfMqezIaTQ6k=; b=memEFYHgHXNpdOEUDeE8tQtfT7FcZ6NXaOkXbGFlvAYDR3uVianW5QkdfwSWE5w4uN N7z2pAQhasij8JT+Eymd73LyB60awwFegnkNz78z8QzSG+9sK25YIS905vMVzp7yFmfm YqbL+vJ2j7c26x4uobycT19SyBtPxtinl3Kr2sU6aEyOHbOVnXt1ir/5ZhB0oPHpEiTN EO8acgSV1C06t6JQBICzU5wt0FXzVwXFDVIuELlAecRKwzqGE38EwleW3wrnEi6JZuFn qhucCPIjdUvCNZvoAgUZ7pTAwwvGEi3kEDHETIkmKo+ohPNQSKhXo74fi33PL8+KnbJQ 60FA== X-Gm-Message-State: AIVw1138mM5P6PctPdBHzavBT1518njXLSbZN0LHhyBCm6fHMia2ckFi 6HYNMMySDcqHFJBy X-Received: by 10.55.1.140 with SMTP id u12mr1249434qkg.108.1499794125721; Tue, 11 Jul 2017 10:28:45 -0700 (PDT) Received: from localhost.localdomain (75-171-229-159.hlrn.qwest.net. [75.171.229.159]) by smtp.gmail.com with ESMTPSA id v68sm364683qkb.65.2017.07.11.10.28.44 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 11 Jul 2017 10:28:45 -0700 (PDT) Subject: Re: [PATCH 0/3] C/C++: show pertinent open token when missing a close token To: David Malcolm , gcc-patches@gcc.gnu.org References: <1499786685-37297-1-git-send-email-dmalcolm@redhat.com> From: Martin Sebor Message-ID: <21581248-3b39-a15c-1381-1860cf1f7244@gmail.com> Date: Tue, 11 Jul 2017 17:28:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1499786685-37297-1-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg00544.txt.bz2 On 07/11/2017 09:24 AM, David Malcolm wrote: > [This patch kit is effectively just one patch; I've split it up into > 3 parts, in the hope of making it easier to review: > the c-family parts, the C parts, and the C++ parts] > > This patch adds a hint to the user to various errors generated > in the C frontend by: > > c_parser_require (parser, CPP_CLOSE_PAREN, "expected %<)%>") > c_parser_skip_until_found (parser, CPP_CLOSE_BRACE, "expected %<}%>") > > etc (where there's a non-NULL msgid), and in the C++ frontend by: > > cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN) > cp_parser_require (parser, CPP_CLOSE_BRACE, RT_CLOSE_BRACE) > > The hint shows the user where the pertinent open paren or open brace > is, which ought to be very helpful for complicated nested collections > of parentheses, and somewhat helpful even for simple cases; I've played with the patch a bit. First, let me say that I like how it associates the curlies. I agree that it will be helpful. There are other cases where highlighting mismatched or missing tokens might be useful, such as for pairs of < and > in complex template declarations. But I mainly experimented with it to see if I could get it to manifest some of the same symptoms I described in bug 81269. I'm not sure it does reproduce the exact same thing or if it's a feature, so let me use this as an opportunity to ask. Given something like namespace { enum { e I see this output: a.C:3:8: error: expected ‘}’ at end of input enum { e ~ ^ a.C:3:8: error: expected unqualified-id at end of input a.C:3:8: error: expected ‘}’ at end of input a.C:1:11: note: to match this ‘{’ namespace { ^ with the first open curly/caret in green, the 'e' in red, and the last open curly/caret in cyan. Is the green color intended? And if yes, what is the intent of distinguishing it from the red 'e'? I note that the caret is red (and there are no other colors in the output) in this case: namespace { enum { but becomes green again when I add an enumerator: namespace { enum { e I ask because in the test case in 81269, highlighting the different tokens in the three colors seems especially confusing and I'd like to better understand if it's intentional (and what it means). Incidentally, I tried to make use of this feature in the middle end (in gimple-ssa-sprintf.c), to achieve the same effect as -Wrestric does, but it led to even stranger-looking results so I went back to using plain old warning. See the attachment to bug 81269: https://gcc.gnu.org/bugzilla/attachment.cgi?id=41660 Martin > consider e.g.: > > ...lots of lines of code... > extern "C" { > ...lots of lines of code... > int test (); > EOF > > where the user currently has to hunt through the source file to find > the unclosed '{': > > test.cc:262:12: error: expected '}' at end of input > int test (); > ^ > > With this patch we tell them: > > test.cc:262:12: error: expected '}' at end of input > int test (); > ^ > test.cc:98:12: note: to match this '{' > extern "C" { > ^ > > The patch avoids using a note if the tokens are on the same line, > highlighting the unclosed open token with an underline: > > test.c:3:32: error: expected ')' before ';' token > return ((b * b) - (4 * a * c); > ~ ^ > > The bulk of the changes in the patch are to the parsers, done using > new classes "matching_braces" and "matching_parens", which stash the > location of the opening token during parsing, so that e.g.: > > if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>")) > return; > ...do stuff... > c_parser_require (parser, CPP_CLOSE_PAREN, "expected %<)%>"); > > becomes: > > matching_parens parens; > if (!parens.require_open (parser)) > return; > ...do stuff... > parens.require_close (parser); > > The exact implementation of these classes varies somewhat between the > C and C++ frontends, to deal with implementation differences between > them (I tried to keep them as similar as possible). > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu; > adds 23 PASS results to gcc.sum; adds 99 PASS results to g++.sum. > > OK for trunk? > > gcc/c-family/c-common.c | 17 +- > gcc/c-family/c-common.h | 3 +- > gcc/c/c-parser.c | 647 ++++++++++------ > gcc/c/c-parser.h | 8 +- > gcc/cp/parser.c | 821 ++++++++++++++------- > gcc/testsuite/c-c++-common/missing-close-symbol.c | 33 + > gcc/testsuite/c-c++-common/missing-symbol.c | 50 ++ > .../g++.dg/diagnostic/unclosed-extern-c.C | 3 + > .../g++.dg/diagnostic/unclosed-function.C | 3 + > .../g++.dg/diagnostic/unclosed-namespace.C | 2 + > gcc/testsuite/g++.dg/diagnostic/unclosed-struct.C | 3 + > gcc/testsuite/g++.dg/parse/pragma2.C | 4 +- > gcc/testsuite/gcc.dg/unclosed-init.c | 3 + > 13 files changed, 1084 insertions(+), 513 deletions(-) > create mode 100644 gcc/testsuite/c-c++-common/missing-close-symbol.c > create mode 100644 gcc/testsuite/c-c++-common/missing-symbol.c > create mode 100644 gcc/testsuite/g++.dg/diagnostic/unclosed-extern-c.C > create mode 100644 gcc/testsuite/g++.dg/diagnostic/unclosed-function.C > create mode 100644 gcc/testsuite/g++.dg/diagnostic/unclosed-namespace.C > create mode 100644 gcc/testsuite/g++.dg/diagnostic/unclosed-struct.C > create mode 100644 gcc/testsuite/gcc.dg/unclosed-init.c >