From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 105732 invoked by alias); 18 Nov 2015 22:25: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 105717 invoked by uid 89); 18 Nov 2015 22:25:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 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; Wed, 18 Nov 2015 22:25:48 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id A9F2BC10044A for ; Wed, 18 Nov 2015 22:25:47 +0000 (UTC) Received: from [10.3.232.44] (vpn-232-44.phx2.redhat.com [10.3.232.44]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tAIMPk1k022611; Wed, 18 Nov 2015 17:25:47 -0500 Message-ID: <1447885546.19594.117.camel@surprise> Subject: Re: [PATCH 01/02] PR/62314: add ability to add fixit-hints From: David Malcolm To: Jeff Law Cc: gcc-patches@gcc.gnu.org Date: Wed, 18 Nov 2015 22:25:00 -0000 In-Reply-To: <564CF44F.4010204@redhat.com> References: <1447173325-48683-1-git-send-email-dmalcolm@redhat.com> <564CF44F.4010204@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-11/txt/msg02280.txt.bz2 On Wed, 2015-11-18 at 14:57 -0700, Jeff Law wrote: > On 11/10/2015 09:35 AM, David Malcolm wrote: > > This patch adds the ability to add "fix-it hints" to a rich_location, > > which will be displayed when the corresponding diagnostic is printed. > > > > It does not actually add any fix-it hints (that comes in the second > > patch), but it adds test coverage of the machinery and printing, > > by using the existing diagnostic_plugin_test_show_locus to inject > > some meaningless fixit hints, and to verify the output. > > > > For now, add a nasty linker kludge in layout::print_any_fixits for > > the sake of diagnostic_plugin_test_show_locus. > > > > Successfully bootstrapped®rtested the pair of patches on > > x86_64-pc-linux-gnu (on top of the 10-patch diagnostics kit). > > > > OK for trunk? > > > > gcc/ChangeLog: > > PR/62314 > > * diagnostic-show-locus.c (colorizer::set_fixit_hint): New. > > (class layout): Update comment > > (layout::print_any_fixits): New method. > > (layout::move_to_column): New method. > > (diagnostic_show_locus): Add call to layout.print_any_fixits. > > > > gcc/testsuite/ChangeLog: > > PR/62314 > > * gcc.dg/plugin/diagnostic-test-show-locus-ascii-bw.c > > (test_fixit_insert): New. > > (test_fixit_remove): New. > > (test_fixit_replace): New. > > * gcc.dg/plugin/diagnostic-test-show-locus-ascii-color.c > > (test_fixit_insert): New. > > (test_fixit_remove): New. > > (test_fixit_replace): New. > > * gcc.dg/plugin/diagnostic_plugin_test_show_locus.c > > (test_show_locus): Add tests of rendering fixit hints. > > > > libcpp/ChangeLog: > > PR/62314 > > * include/line-map.h (source_range::intersects_line_p): New > > method. > > (rich_location::~rich_location): New. > > (rich_location::add_fixit_insert): New method. > > (rich_location::add_fixit_remove): New method. > > (rich_location::add_fixit_replace): New method. > > (rich_location::get_num_fixit_hints): New accessor. > > (rich_location::get_fixit_hint): New accessor. > > (rich_location::MAX_FIXIT_HINTS): New constant. > > (rich_location::m_num_fixit_hints): New field. > > (rich_location::m_fixit_hints): New field. > > (class fixit_hint): New class. > > (class fixit_insert): New class. > > (class fixit_remove): New class. > > (class fixit_replace): New class. > > * line-map.c (source_range::intersects_line_p): New method. > > (rich_location::rich_location): Add initialization of > > m_num_fixit_hints to both ctors. > > (rich_location::~rich_location): New. > > (rich_location::add_fixit_insert): New method. > > (rich_location::add_fixit_remove): New method. > > (rich_location::add_fixit_replace): New method. > > (fixit_insert::fixit_insert): New. > > (fixit_insert::~fixit_insert): New. > > (fixit_insert::affects_line_p): New. > > (fixit_remove::fixit_remove): New. > > (fixit_remove::affects_line_p): New. > > (fixit_replace::fixit_replace): New. > > (fixit_replace::~fixit_replace): New. > > (fixit_replace::affects_line_p): New. > > > + > > + /* Nasty workaround to convince the linker to add > > + rich_location::add_fixit_insert > > + rich_location::add_fixit_remove > > + rich_location::add_fixit_replace > > + to cc1 for use by diagnostic_plugin_test_show_locus, > > + before anything in cc1 is using them. > > + > > + This conditional should never hold, but hopefully the compiler can't > > + figure that out. */ > > + if ('.' == global_dc->caret_chars[0]) > > + { > > + rich_location dummy (line_table, UNKNOWN_LOCATION); > > + dummy.add_fixit_insert (UNKNOWN_LOCATION, ""); > > + dummy.add_fixit_remove > > + (source_range::from_location (UNKNOWN_LOCATION)); > > + dummy.add_fixit_replace > > + (source_range::from_location (UNKNOWN_LOCATION), ""); > > + } > > +} > So "the kludge" is presumably going to be removed based on later > comments in this patch's thread. Yes. > What is the purpose of the #if 0 code in the various tests? Did you > mean to leave those in? Presumably you're referring to the bodies of the functions test_fixit_insert test_fixit_remove test_fixit_replace within: gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color.c where the bodies are purely of the form: { #if 0 some code, containing dejagnu directives. #endif } Although this looks weird, it's deliberate, and follows the pattern earlier in those test files: the diagnostics are injected by the plugin, not by cc1. The plugin gives us a way of unit-testing how diagnostic_show_locus handles the various ways of calling into the diagnostic API, isolating it from the details of any particular real diagnostic in cc1, and from the details of how to get real location/range information. Hence we inject calls to the diagnostic API via the plugin, and the bodies of the function could be anything - but I chose them to give representative-looking diagnostics. In fact, in test_caret_on_leading_whitespace we have a fragment of Fortran code in a C file (since this was a regression test for an issue I saw printing Fortran diagnostics during development of the new diagnostic_show_locus). Hope this makes sense. (FWIW there's a comment about this at the top of diagnostic_plugin_test_show_locus.c, though of course that's not visible in the patch under review). Hence the testcase gives some examples of what uses of the 3 kinds of fix-its might look like. To actually implement those fix-its for those diagnostics would be separate patches (fwiw I have some followups already posted that update the levenshtein/spelling suggestion thing to issue fix-its for the suggestions, but they'll have bit-rotted; I'll update them and repost them). (Ultimately we could have some kind of option to emit the fixit in machine-parsable form, so that e.g. an IDE can offer to directly apply the change; again, I see that as a followup). > I think this is probably OK. Though I am concerned that with blobs of > the tests commented out that it's not being tested as thoroughly as you > think it has :-) Does the above address your concerns? (Joesph already approved the other patch) Dave