From patchwork Fri Sep 20 20:45:47 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 1165440 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-509378-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="A5yutofu"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 46Zm2h4Wggz9s00 for ; Sat, 21 Sep 2019 06:46:01 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id; q=dns; s=default; b=xakXAfLfbWXU EDPAxTL+7hPwiQ/JSsqgX9rIUngMKkw0EUQhUkWUfQ1hLDcZXdpHJVQqxokdcXI1 Q90iAfRQ5ycNQRsm7QbnClmfDu3+2mtPgzsjfi3cvvTHqcKQ3ueETNFssXT/KqVA NdYno7HCys39eN+62+AG/4WUWvqAr98= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id; s=default; bh=IJ0QATnCUX0yjJijBi 08W/AvNRs=; b=A5yutofuLx8JFZY5NeLVusbNAj+pGU5NpRwO86bZ4xJ/wYSQHE wPXCd5mZieE3bzzL9MbWCVFT+EwWt4hAFEyxgtQ4jY5jWccDXQEBcp3EUl+MPV7V 2CEhEi/qt2CiV4sNYPIILkI9GRjhV0cgXs1w7G8YcRin/WCTLhjf1Y+4g= Received: (qmail 54141 invoked by alias); 20 Sep 2019 20:45:52 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 54127 invoked by uid 89); 20 Sep 2019 20:45:52 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=Duplicate, tweaks, colored 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 ESMTP; Fri, 20 Sep 2019 20:45:51 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DA3DEC057F31; Fri, 20 Sep 2019 20:45:49 +0000 (UTC) Received: from t470.redhat.com (ovpn-117-236.phx2.redhat.com [10.3.117.236]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2A45E6061E; Fri, 20 Sep 2019 20:45:49 +0000 (UTC) From: David Malcolm To: gcc-patches@gcc.gnu.org, fortran@gcc.gnu.org Cc: David Malcolm Subject: [PATCH] PR fortran/91426: Colorize %L text to match diagnostic_show_locus Date: Fri, 20 Sep 2019 16:45:47 -0400 Message-Id: <20190920204547.13300-1-dmalcolm@redhat.com> X-IsSubscribed: yes PR fortran/91426 reports that the following program program main 10 continue 10 continue end yields: label.f90:2:2: 2 | 10 continue | 1 3 | 10 continue | 2 Error: Duplicate statement label 10 at (1) and (2) where the '1' and '2' annotating lines 2 and 3 in the source are colored red and green respectively, but the "(1)" and "(2)" in the "Error" line are not colored - only the word "Error" in that line is colored (red). PR fortran/91426 covers this inconsistency, and there was some debate there on ways we could address it. Within the diagnostics subsystem as of GCC 6 onwards, a rich_location can contain multiple location_t values. The first is considered the "primary" location, subsequent ones are considered "secondary" locations. diagnostic_show_locus colorizes locations in the annotated source lines by using the "diagnostic_kind" color for the primary location_t within the rich_location, and then alternating between two other colors for any secondary location_t values within the rich_location ("range1" and "range2" within GCC_COLORS). fortran/error.c currently supports at most 2 locations per diagnostic (via the %L formatting code). Hence fortran diagnostics have a primary location_t, and some have a single secondary location_t. Based on discussion with tkoenig in the bug report, this patch tweaks fortran's error.c so that it colorizes the "(1)" and "(2)" in the "Error" line, with their colors matching those of the "1" and "2" in the source line annotations. For a single-%L diagnostic, it colorizes the single "(1)" so that its color matches that of the "1" in the source line annotation. My view is that this makes the overall output more readable, making the association between the text of the diagnostic and the annotated source clearer. For example, if you have multiple diagnostics with a mixture of warnings and errors, the differences in colorization make it clearer (to me, at least) what's being referred to in the text of each diagnostic (e.g. I've been testing with gfortran.dg/achar_3.f90 -Wall). As normal, no colorization is done if colorization is disabled e.g. via '-fdiagnostics-color=never' or if stderr isn't going to a tty. Successfully bootstrapped®rtested on x86_64-pc-linux-gnu. I think technically I can self-approve this, but I'm not a day-to-day user of fortran; does this look sane? Thanks David gcc/fortran/ChangeLog: PR fortran/91426 * error.c (curr_diagnostic): New static variable. (gfc_report_diagnostic): New static function. (gfc_warning): Replace call to diagnostic_report_diagnostic with call to gfc_report_diagnostic. (gfc_format_decoder): Colorize the text of %L and %C to match the colorization used by diagnostic_show_locus. (gfc_warning_now_at): Replace call to diagnostic_report_diagnostic with call to gfc_report_diagnostic. (gfc_warning_now): Likewise. (gfc_warning_internal): Likewise. (gfc_error_now): Likewise. (gfc_fatal_error): Likewise. (gfc_error_opt): Likewise. (gfc_internal_error): Likewise. --- gcc/fortran/error.c | 44 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/gcc/fortran/error.c b/gcc/fortran/error.c index 68a2791..a0ce7a6 100644 --- a/gcc/fortran/error.c +++ b/gcc/fortran/error.c @@ -760,6 +760,23 @@ gfc_clear_pp_buffer (output_buffer *this_buffer) global_dc->last_location = UNKNOWN_LOCATION; } +/* The currently-printing diagnostic, for use by gfc_format_decoder, + for colorizing %C and %L. */ + +static diagnostic_info *curr_diagnostic; + +/* A helper function to call diagnostic_report_diagnostic, while setting + curr_diagnostic for the duration of the call. */ + +static bool +gfc_report_diagnostic (diagnostic_info *diagnostic) +{ + gcc_assert (diagnostic != NULL); + curr_diagnostic = diagnostic; + bool ret = diagnostic_report_diagnostic (global_dc, diagnostic); + curr_diagnostic = NULL; + return ret; +} /* This is just a helper function to avoid duplicating the logic of gfc_warning. */ @@ -789,7 +806,7 @@ gfc_warning (int opt, const char *gmsgid, va_list ap) diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc, DK_WARNING); diagnostic.option_index = opt; - bool ret = diagnostic_report_diagnostic (global_dc, &diagnostic); + bool ret = gfc_report_diagnostic (&diagnostic); if (buffered_p) { @@ -954,7 +971,18 @@ gfc_format_decoder (pretty_printer *pp, text_info *text, const char *spec, loc->lb->location, offset); text->set_location (loc_num, src_loc, SHOW_RANGE_WITH_CARET); + /* Colorize the markers to match the color choices of + diagnostic_show_locus (the initial location has a color given + by the "kind" of the diagnostic, the secondary location has + color "range1"). */ + gcc_assert (curr_diagnostic != NULL); + const char *color + = (loc_num + ? "range1" + : diagnostic_get_color_for_kind (curr_diagnostic->kind)); + pp_string (pp, colorize_start (pp_show_color (pp), color)); pp_string (pp, result[loc_num]); + pp_string (pp, colorize_stop (pp_show_color (pp))); return true; } default: @@ -1153,7 +1181,7 @@ gfc_warning_now_at (location_t loc, int opt, const char *gmsgid, ...) va_start (argp, gmsgid); diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc, DK_WARNING); diagnostic.option_index = opt; - ret = diagnostic_report_diagnostic (global_dc, &diagnostic); + ret = gfc_report_diagnostic (&diagnostic); va_end (argp); return ret; } @@ -1172,7 +1200,7 @@ gfc_warning_now (int opt, const char *gmsgid, ...) diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc, DK_WARNING); diagnostic.option_index = opt; - ret = diagnostic_report_diagnostic (global_dc, &diagnostic); + ret = gfc_report_diagnostic (&diagnostic); va_end (argp); return ret; } @@ -1191,7 +1219,7 @@ gfc_warning_internal (int opt, const char *gmsgid, ...) diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc, DK_WARNING); diagnostic.option_index = opt; - ret = diagnostic_report_diagnostic (global_dc, &diagnostic); + ret = gfc_report_diagnostic (&diagnostic); va_end (argp); return ret; } @@ -1209,7 +1237,7 @@ gfc_error_now (const char *gmsgid, ...) va_start (argp, gmsgid); diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc, DK_ERROR); - diagnostic_report_diagnostic (global_dc, &diagnostic); + gfc_report_diagnostic (&diagnostic); va_end (argp); } @@ -1225,7 +1253,7 @@ gfc_fatal_error (const char *gmsgid, ...) va_start (argp, gmsgid); diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc, DK_FATAL); - diagnostic_report_diagnostic (global_dc, &diagnostic); + gfc_report_diagnostic (&diagnostic); va_end (argp); gcc_unreachable (); @@ -1310,7 +1338,7 @@ gfc_error_opt (int opt, const char *gmsgid, va_list ap) } diagnostic_set_info (&diagnostic, gmsgid, &argp, &richloc, DK_ERROR); - diagnostic_report_diagnostic (global_dc, &diagnostic); + gfc_report_diagnostic (&diagnostic); if (buffered_p) { @@ -1360,7 +1388,7 @@ gfc_internal_error (const char *gmsgid, ...) va_start (argp, gmsgid); diagnostic_set_info (&diagnostic, gmsgid, &argp, &rich_loc, DK_ICE); - diagnostic_report_diagnostic (global_dc, &diagnostic); + gfc_report_diagnostic (&diagnostic); va_end (argp); gcc_unreachable ();