Patchwork [PR,preprocessor/42014] Added LAST_SOURCE_COLUMN in while loop

login
register
mail settings
Submitter Shakthi Kannan
Date May 10, 2013, 6:55 a.m.
Message ID <1727363575.9580702.1368168907851.JavaMail.root@redhat.com>
Download mbox | patch
Permalink /patch/242917/
State New
Headers show

Comments

Shakthi Kannan - May 10, 2013, 6:55 a.m.
Hi,

The attached patch adds LAST_SOURCE_COLUMN to pp_verbatim
function in the while loop present in
diagnostic_report_current_module(). This makes the output
consistent for any error parsing program as stated in the bug.

2013-05-10 Shakthi Kannan <skannan@redhat.com>

    PR preprocessor/42014
    * gcc/diagnostic.c: Added LAST_SOURCE_COLUMN in while loop.

---
 gcc/diagnostic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Marek Polacek - May 10, 2013, 7:24 a.m.
On Fri, May 10, 2013 at 02:55:07AM -0400, Shakthi Kannan wrote:
> Hi,
> 
> The attached patch adds LAST_SOURCE_COLUMN to pp_verbatim
> function in the while loop present in
> diagnostic_report_current_module(). This makes the output
> consistent for any error parsing program as stated in the bug.
> 
> 2013-05-10 Shakthi Kannan <skannan@redhat.com>
> 
>     PR preprocessor/42014
>     * gcc/diagnostic.c: Added LAST_SOURCE_COLUMN in while loop.

Maybe
	PR preprocessor/42014
	* diagnostic.c: Print LAST_SOURCE_COLUMN as well.
?

	Marek
Shakthi Kannan - May 10, 2013, 9:26 a.m.
Hi,

----- Original Message -----
| From: "Marek Polacek" <polacek@redhat.com>
|
| Maybe
|	PR preprocessor/42014
|	* diagnostic.c: Print LAST_SOURCE_COLUMN as well.
| ?
\--

Sure!

SK
Tom Tromey - May 10, 2013, 4:26 p.m.
>>>>> "Shakthi" == Shakthi Kannan <skannan@redhat.com> writes:

Shakthi> 2013-05-10 Shakthi Kannan <skannan@redhat.com>
Shakthi>     PR preprocessor/42014
Shakthi>     * gcc/diagnostic.c: Added LAST_SOURCE_COLUMN in while loop.

You should mention the function name in there.
See the GNU Coding Standards for the format.

Shakthi> -			   ",\n                 from %s:%d",
Shakthi> -			   LINEMAP_FILE (map), LAST_SOURCE_LINE (map));
Shakthi> +			   ",\n                 from %s:%d:%d",
Shakthi> +			   LINEMAP_FILE (map),
Shakthi> +			   LAST_SOURCE_LINE (map), LAST_SOURCE_COLUMN (map));

Does this cause test suite regressions?

If so then the patch needs fixes there.
If not then the patch needs a new test.

Also, is the column number actually correct?
IIRC some things in cpp still don't get the right column number.

Tom
Shakthi Kannan - May 15, 2013, 10:22 a.m.
Hi,

----- Original Message -----
| From: "Tom Tromey" <tromey@redhat.com>
|
| Does this cause test suite regressions?
\--

I built gcc-4.8.0 with and without the patch, and ran the test suite for both instances using:

  $ make -k check

There was no difference in the test result output. Is this the same as the test suite regression?

---
| If so then the patch needs fixes there.
| If not then the patch needs a new test.
\--

Sorry, where should this new test be written, and what does it need to test?

Thanks for your reply.

SK
Mike Stump - May 15, 2013, 5:37 p.m.
On May 15, 2013, at 3:22 AM, Shakthi Kannan <skannan@redhat.com> wrote:
> ----- Original Message -----
> | From: "Tom Tromey" <tromey@redhat.com>
> |
> | Does this cause test suite regressions?
> \--
> 
> I built gcc-4.8.0 with and without the patch, and ran the test suite for both instances using:
> 
>  $ make -k check
> 
> There was no difference in the test result output.

The output of make -k, or the contents of all the .sum and .log files, or…  For example, if you compared all the .sum files, and there were no differences (literally), then you did something wrong, at least the dates must differ.

> Is this the same as the test suite regression?

I like using ~/contrib/compare_tests gcc-before.sum gcc-after.sum to determine if there are regressions.  You can also use that script to check for regressions between two build trees as well.  The output is priority sorted with the important stuff first, and lessor things later.  regression is defined as that script returning 1, and no regression is if the script returns 0.  Anyway, life is more complicated than this, but, it is a good first order approximation.
Shakthi Kannan - May 16, 2013, 5:41 a.m.
Hi,

----- Original Message -----
| From: "Mike Stump" <mrs@mrs.kithrup.com>
|
| The output of make -k, or the contents of all the .sum and .log files, 
\--

The output of "make -k". I did a "grep '^#'" on the output and the following results were the same, with and without the patch applied:

  # of expected passes		96818
  # of unexpected failures	19
  # of unexpected successes	39
  # of expected failures	267
  # of unsupported tests	1400
  # of expected passes		54561
  # of expected failures	294
  # of unsupported tests	916
  # of expected passes		44247
  # of unexpected failures	6
  # of expected failures	56
  # of unresolved testcases	6
  # of unsupported tests	71
  # of expected passes		2988
  # of expected failures	6
  # of unsupported tests	74
  # of expected passes		9340
  # of unexpected failures	4
  # of expected failures	45
  # of unsupported tests	212
  # of expected passes		1433
  # of unexpected failures	3
  # of expected passes		1819
  # of unsupported tests	55
  # of expected passes		2582
  # of unexpected failures	4
  # of expected passes		12
  # of unsupported tests	1
  # of expected passes		2998
  # of expected passes		26
  # of expected failures	3
  # of unsupported tests	1
  # of expected passes		54

---
| I like using ~/contrib/compare_tests gcc-before.sum gcc-after.sum to 
| determine if there are regressions.  You can also use that script to
| check for regressions between two build trees as well.  
\--

I ran the the script between the two build trees. Here is the output:

  $ ./without/gcc/contrib/compare_tests ./without/build ./with/build
  # Comparing directories
  ## Dir1=./without/build: 12 sum files
  ## Dir2=./with/build: 12 sum files

  # Comparing 12 common sum files
  ## /bin/sh ./without/gcc/contrib/compare_tests  /tmp/gxx-sum1.2065 /tmp/gxx-sum2.2065
  # No differences found in 12 common sum files

Regards,

SK
Mike Stump - May 16, 2013, 12:25 p.m.
On May 15, 2013, at 10:41 PM, Shakthi Kannan <skannan@redhat.com> wrote:
> | I like using ~/contrib/compare_tests gcc-before.sum gcc-after.sum to 
> | determine if there are regressions.  You can also use that script to
> | check for regressions between two build trees as well.  
> \--
> 
> I ran the the script between the two build trees. Here is the output:
> 
>  $ ./without/gcc/contrib/compare_tests ./without/build ./with/build
>  # Comparing directories
>  ## Dir1=./without/build: 12 sum files
>  ## Dir2=./with/build: 12 sum files
> 
>  # Comparing 12 common sum files
>  ## /bin/sh ./without/gcc/contrib/compare_tests  /tmp/gxx-sum1.2065 /tmp/gxx-sum2.2065
>  # No differences found in 12 common sum files

:-)  That's two thumbs up.

Patch

diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index f9a236b..2addbf0 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -528,8 +528,9 @@  diagnostic_report_current_module (diagnostic_context *context, location_t where)
 	    {
 	      map = INCLUDED_FROM (line_table, map);
 	      pp_verbatim (context->printer,
-			   ",\n                 from %s:%d",
-			   LINEMAP_FILE (map), LAST_SOURCE_LINE (map));
+			   ",\n                 from %s:%d:%d",
+			   LINEMAP_FILE (map),
+			   LAST_SOURCE_LINE (map), LAST_SOURCE_COLUMN (map));
 	    }
 	  pp_verbatim (context->printer, ":");
 	  pp_newline (context->printer);