Remove LINEMAP_POSITION_FOR_COLUMN macro

[Gabriel Charette, 2011-08-11 20:52:03]

Removed LINEMAP_POSITION_FOR_COLUMN, it did the EXACT same thing as
linemap_position_for_column, so maintaining both in parallel seems like overkill
to me. The only thing I can think of is that it's more optimal as it's inlined
(but if that's really needed we can always make linemap_position_for_column an
inline function directly).

Tested with full boostrap and regression testing with all+go language
configuration.

Ok for trunk?

Gabriel

2011-08-11  Gabriel Charette  <gchare@google.com>

	* include/line-map.h (LINEMAP_POSITION_FOR_COLUMN): Remove.
	Update all users to use linemap_position_for_column instead.

diff --git a/gcc/go/gofrontend/lex.cc b/gcc/go/gofrontend/lex.cc
index 9f26911..167c7dd 100644
--- a/gcc/go/gofrontend/lex.cc
+++ b/gcc/go/gofrontend/lex.cc
@@ -518,9 +518,7 @@ Lex::require_line()
 source_location
 Lex::location() const
 {
-  source_location location;
-  LINEMAP_POSITION_FOR_COLUMN(location, line_table, this->lineoff_ + 1);
-  return location;
+  return linemap_position_for_column (line_table, this->lineoff_ + 1);
 }
 
 // Get a location slightly before the current one.  This is used for
@@ -529,9 +527,7 @@ Lex::location() const
 source_location
 Lex::earlier_location(int chars) const
 {
-  source_location location;
-  LINEMAP_POSITION_FOR_COLUMN(location, line_table, this->lineoff_ + 1 -
chars);
-  return location;
+  return linemap_position_for_column (line_table, this->lineoff_ + 1 - chars);
 }
 
 // Get the next token.
diff --git a/libcpp/directives-only.c b/libcpp/directives-only.c
index e19f806..c6772af 100644
--- a/libcpp/directives-only.c
+++ b/libcpp/directives-only.c
@@ -142,7 +142,7 @@ _cpp_preprocess_dir_only (cpp_reader *pfile,
 	    flags |= DO_LINE_COMMENT;
 	  else if (!(flags & DO_SPECIAL))
 	    /* Mark the position for possible error reporting. */
-	    LINEMAP_POSITION_FOR_COLUMN (loc, pfile->line_table, col);
+	    loc = linemap_position_for_column (pfile->line_table, col);
 
 	  break;
 
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index f1d5bee..3c84035 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -165,23 +165,6 @@ extern const struct line_map *linemap_lookup
 /* Nonzero if the map is at the bottom of the include stack.  */
 #define MAIN_FILE_P(MAP) ((MAP)->included_from < 0)
 
-/* Set LOC to a source position that is the same line as the most recent
-   linemap_line_start, but with the specified TO_COLUMN column number.  */
-
-#define LINEMAP_POSITION_FOR_COLUMN(LOC, SET, TO_COLUMN) do { \
-  unsigned int to_column = (TO_COLUMN); \
-  struct line_maps *set = (SET); \
-  if (__builtin_expect (to_column >= set->max_column_hint, 0)) \
-    (LOC) = linemap_position_for_column (set, to_column); \
-  else { \
-    source_location r = set->highest_line; \
-    r = r + to_column; \
-    if (r >= set->highest_location) \
-      set->highest_location = r; \
-    (LOC) = r;			 \
-  }} while (0)
-    
-
 extern source_location
 linemap_position_for_column (struct line_maps *set, unsigned int to_column);
 
diff --git a/libcpp/lex.c b/libcpp/lex.c
index d29f36d..d460b98 100644
--- a/libcpp/lex.c
+++ b/libcpp/lex.c
@@ -1975,8 +1975,8 @@ _cpp_lex_direct (cpp_reader *pfile)
     }
   c = *buffer->cur++;
 
-  LINEMAP_POSITION_FOR_COLUMN (result->src_loc, pfile->line_table,
-			       CPP_BUF_COLUMN (buffer, buffer->cur));
+  result->src_loc = linemap_position_for_column (pfile->line_table,
+			                                    CPP_BUF_COLUMN (buffer, buffer->cur));
 
   switch (c)
     {

--
This patch is available for review at http://codereview.appspot.com/4874043

[tromey_redhat.com, 2011-08-12 18:27:17]

>>>>> "Gabriel" == Gabriel Charette <gchare@google.com> writes:

Gabriel> Removed LINEMAP_POSITION_FOR_COLUMN, it did the EXACT same
Gabriel> thing as linemap_position_for_column, so maintaining both in
Gabriel> parallel seems like overkill to me. The only thing I can think
Gabriel> of is that it's more optimal as it's inlined (but if that's
Gabriel> really needed we can always make linemap_position_for_column an
Gabriel> inline function directly).

I am sympathetic to this, but nobody is likely to do the performance
tests post-patch.

The libcpp parts are ok if it doesn't cause a noticeable slowdown in the
GCC bootstrap.

I can't approve the changes outside of libcpp.
Also, you didn't write a ChangeLog entry for those.

Tom

[Gabriel Charette, 2011-08-15 18:05:51]

[+iant]

Ian: can you approve the go changes in this patch?

It's a change in the linemap that reflects in functions used by go,
but that shouldn't have any effect on the go compiler's behaviour.

As a side thought, I'm getting random failures in the go test suite,
in my last run the difference was that
go.go-torture/execute/select-1.go failed in the clean build test and
passed in the patched build...

Every full test run I do I get different sets of differences, I've had
identical runs too, I highly doubt this is due to my patch.

On Fri, Aug 12, 2011 at 11:27 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Gabriel" == Gabriel Charette <gchare@google.com> writes:
>
> Gabriel> Removed LINEMAP_POSITION_FOR_COLUMN, it did the EXACT same
> Gabriel> thing as linemap_position_for_column, so maintaining both in
> Gabriel> parallel seems like overkill to me. The only thing I can think
> Gabriel> of is that it's more optimal as it's inlined (but if that's
> Gabriel> really needed we can always make linemap_position_for_column an
> Gabriel> inline function directly).
>
> I am sympathetic to this, but nobody is likely to do the performance
> tests post-patch.
>
> The libcpp parts are ok if it doesn't cause a noticeable slowdown in the
> GCC bootstrap.
>

I just did performance test and the results were identical before and after.

> I can't approve the changes outside of libcpp.
> Also, you didn't write a ChangeLog entry for those.
>

Here is the ChangeLog including gcc/go changes, added iant to get
approval for the go changes:

2011-08-11  Gabriel Charette  <gchare@google.com>

        libcpp/ChangeLog
	* include/line-map.h (LINEMAP_POSITION_FOR_COLUMN): Remove.
	Update all users to use linemap_position_for_column instead.

        gcc/go/ChangeLog
	* gofrontend/lex.cc (Lex::location): Update to use
	linemap_position_for_column instead.
        (Lex::earlier_location): Likewise.


Thanks,
Gabriel

[Diego Novillo, 2011-08-15 19:50:47]

On 2011/08/15 18:05:51, Gabriel Charette wrote:

> 2011-08-11  Gabriel Charette  <mailto:gchare@google.com>
> 
>         libcpp/ChangeLog
> 	* include/line-map.h (LINEMAP_POSITION_FOR_COLUMN): Remove.
> 	Update all users to use linemap_position_for_column instead.
> 
>         gcc/go/ChangeLog
> 	* gofrontend/lex.cc (Lex::location): Update to use
> 	linemap_position_for_column instead.
>         (Lex::earlier_location): Likewise.

This is OK.  The changes to Go are just a side-effect of the other changes and
they are small enough that they should be easy to rollback.


Diego.

[iant2, 2011-08-15 20:39:59]

Gabriel Charette <gchare@google.com> writes:

> Ian: can you approve the go changes in this patch?

The changes to the Go frontend are fine, but they need to be applied to
the master repository for the Go frontend.  I can take care of that when
you are ready to commit the patch.  Note that changes to the files in
gcc/go/gofrontend do not get ChangeLog entries.


> As a side thought, I'm getting random failures in the go test suite,
> in my last run the difference was that
> go.go-torture/execute/select-1.go failed in the clean build test and
> passed in the patched build...
>
> Every full test run I do I get different sets of differences, I've had
> identical runs too, I highly doubt this is due to my patch.

I'm sure it has nothing to do with your patch.  I have not seen other
people report this, though.  Some of the tests in the Go testsuite use a
lot of threads, like thousands.  It's possible that that is running into
some limit on your machine.  This would be particularly applicable if
you are running the testsuite with make -j.

Ian

[Gabriel Charette, 2011-08-15 21:04:55]

On Mon, Aug 15, 2011 at 1:39 PM, Ian Lance Taylor <iant@google.com> wrote:
> Gabriel Charette <gchare@google.com> writes:
>
>> Ian: can you approve the go changes in this patch?
>
> The changes to the Go frontend are fine, but they need to be applied to
> the master repository for the Go frontend.  I can take care of that when
> you are ready to commit the patch.  Note that changes to the files in
> gcc/go/gofrontend do not get ChangeLog entries.
>

The patch has been committed to trunk.

I did also provide a ChangeLog for the gcc/go/gofrontend change, want
me to remove it?

>
>> As a side thought, I'm getting random failures in the go test suite,
>> in my last run the difference was that
>> go.go-torture/execute/select-1.go failed in the clean build test and
>> passed in the patched build...
>>
>> Every full test run I do I get different sets of differences, I've had
>> identical runs too, I highly doubt this is due to my patch.
>
> I'm sure it has nothing to do with your patch.  I have not seen other
> people report this, though.  Some of the tests in the Go testsuite use a
> lot of threads, like thousands.  It's possible that that is running into
> some limit on your machine.  This would be particularly applicable if
> you are running the testsuite with make -j.
>

Ok (and yes I am using make -j)

Gabriel

[iant2, 2011-08-16 04:41:11]

Gabriel Charette <gchare@google.com> writes:

> On Mon, Aug 15, 2011 at 1:39 PM, Ian Lance Taylor <iant@google.com> wrote:
>> Gabriel Charette <gchare@google.com> writes:
>>
>>> Ian: can you approve the go changes in this patch?
>>
>> The changes to the Go frontend are fine, but they need to be applied to
>> the master repository for the Go frontend.  I can take care of that when
>> you are ready to commit the patch.  Note that changes to the files in
>> gcc/go/gofrontend do not get ChangeLog entries.
>>
>
> The patch has been committed to trunk.

I committed the patch to the master repository.

> I did also provide a ChangeLog for the gcc/go/gofrontend change, want
> me to remove it?

I took care of it.

Thanks for updating the frontend.

Ian