On 11/12/18 7:56 PM, David Malcolm wrote:
> On Mon, 2018-11-12 at 21:13 +0000, Mike Gulick wrote:
>> On 11/2/18 5:04 PM, David Malcolm wrote:
>>> On Thu, 2018-11-01 at 11:56 -0400, Mike Gulick wrote:
>>>> 2017-10-31 Mike Gulick <[email protected]>
>>>>
>>>> PR preprocessor/83173
>>>> * gcc/input.c (dump_location_info): Dump reason and
>>>> included_from fields from line_map_ordinary struct. Fix
>>>> indentation when location > 5 digits.
>>>>
>>>> * libcpp/location-example.txt: Update example
>>>> -fdump-internal-locations output.
>>>> ---
>>>> gcc/input.c | 49 +++++-
>>>> libcpp/location-example.txt | 333 +++++++++++++++++++++---------
>>>> ----
>>>> --
>>>> 2 files changed, 241 insertions(+), 141 deletions(-)
>>>
>>> Sorry about the belated response. This is a nice enhancement; some
>>> nits below.
>>>
>>>> diff --git a/gcc/input.c b/gcc/input.c
>>>> index a94a010f353..f938a37f20e 100644
>>>> --- a/gcc/input.c
>>>> +++ b/gcc/input.c
>>>> @@ -1075,6 +1075,17 @@ dump_labelled_location_range (FILE
>>>> *stream,
>>>> fprintf (stream, "\n");
>>>> }
>>>>
>>>> +#define NUM_DIGITS(x) ((x) >= 1000000000 ? 10 : \
>>>> + (x) >= 100000000 ? 9 : \
>>>> + (x) >= 10000000 ? 8 : \
>>>> + (x) >= 1000000 ? 7 : \
>>>> + (x) >= 100000 ? 6 : \
>>>> + (x) >= 10000 ? 5 : \
>>>> + (x) >= 1000 ? 4 : \
>>>> + (x) >= 100 ? 3 : \
>>>> + (x) >= 10 ? 2 : \
>>>> + 1)
>>>
>>> diagnostic-show-locus.c has a function "num_digits" (currently
>>> static)
>>> and, fwiw, a unit test. It would be good to share the
>>> implementation.
>>>
>>
>> I initially tried to use this function by just adding "extern int
>> num_digits(int);" into diagnostic-core.h, but that failed to link, so
>> it seems
>> like diagnostic-show-locus.c is not included in whatever library
>> input.c gets
>> linked with (I forget which library it was trying to link).
>
> Both input.o and diagnostic-show-locus.o are in OBJS-libcommon, so I'm
> not sure what went wrong.
>
After looking at libcommon.a with nm, I realized that
diagnostic-show-locus.c wrapped everything inside an anonymous
namespace, so that was why the symbol wasn't visible.
>> Instead I moved
>> num_digits and its unit test to diagnostic.c, and added the extern
>> definition to
>> diagnostic-core.h. That builds and tests successfully. Does that
>> seem like a
>> reasonable way to do this?
>
> Thanks. That sounds good (maybe put the decl in diagnostic.h rather
> than diagnostic-core.h; the latter is used in lots of places, whereas
> the former is more about implementation details).
>
No problem.
>>>> /* Write a visualization of the locations in the line_table to
>>>> STREAM. */
>>>>
>>>> void
>>>> @@ -1104,6 +1115,35 @@ dump_location_info (FILE *stream)
>>>> map->m_column_and_range_bits - map-
>>>>> m_range_bits);
>>>> fprintf (stream, " range bits: %i\n",
>>>> map->m_range_bits);
>>>> + const char * reason;
>>>> + switch (map->reason) {
>>>> + case LC_ENTER:
>>>> + reason = "LC_ENTER";
>>>> + break;
>>>> + case LC_LEAVE:
>>>> + reason = "LC_LEAVE";
>>>> + break;
>>>> + case LC_RENAME:
>>>> + reason = "LC_RENAME";
>>>> + break;
>>>> + case LC_RENAME_VERBATIM:
>>>> + reason = "LC_RENAME_VERBATIM";
>>>> + break;
>>>> + case LC_ENTER_MACRO:
>>>> + reason = "LC_RENAME_MACRO";
>>>> + break;
>>>> + default:
>>>> + reason = "Unknown";
>>>> + }
>>>> + fprintf (stream, " reason: %d (%s)\n", map->reason,
>>>> reason);
>>>> +
>>>> + const line_map_ordinary *includer_map
>>>> + = linemap_included_from_linemap (line_table, map);
>>>> + fprintf (stream, " included from map: %d\n",
>>>> + includer_map ? int (includer_map - line_table-
>>>>> info_ordinary.maps)
>>>>
>>>> + : -1);
>>>
>>> I'm not a fan of "-1" here; it's a NULL pointer in the original
>>> data.
>>> How about "n/a" for that case?
>>>
>>
>> That's a good suggestion. Thanks.
>>
>>>> + fprintf (stream, " included from location: %d\n",
>>>> + linemap_included_from (map));
>>>
>>> ...or merging it with this line, for something like:
>>>
>>> included from location: 127 (in ordinary map 2)
>>>
>>> vs:
>>>
>>> included from location: 0
>>>
>>> [...snip...]
>>>
>>> Other than that, this is OK for trunk, assuming your contributor
>>> paperwork is in place.
>>>
>>> Dave
>>>
>>
>> What is the preferred way to re-send this patch? Should I re-send
>> the entire
>> patch series as v4, or just an updated version of this single patch?
>
> The latter: just an updated version of the changed patch. IIRC the
> rest is all approved.
>
Thanks, I'll reply with the updated patch.
>>
>> Also, I'm waiting on FSF for assignment paperwork. I've re-pinged
>> them after
>> waiting a week.
>
> Thanks.
>
>> Thanks for the feedback and help.
>>
>> -Mike
>
