Re: PATCH to use cpplib's source_location for location_t

[Per Bothner <per at bothner dot com>]

Richard Henderson wrote:

> On Mon, Jun 28, 2004 at 02:55:24PM -0700, Per Bothner wrote:
>
> > +#ifdef __GNUC__
> > +#define expand_location(SOURCE_LOCATION) __extension__ ({ \
> > +  struct location_s __t; \
> > +  source_location __line = (SOURCE_LOCATION); \
> > +  if (__line == 0) { __t.file = NULL; __t.line = 0; } \
> > +  else { \
> > +    const struct line_map *__map = linemap_lookup (&line_table,__line);	\
> > +    __t.file = __map->to_file; \
> > +    __t.line = SOURCE_LINE (__map, __line); \
> > +  };\
> > +  __t; })
> > +#else
> > +expanded location
> > +expand_location (source_location fline)

> Typos.

Could you be more specific?  I see a missing space before __line,
but are there more?

> Any reason to have this inline at all, GCC or otherwise?

If it's not inline, where do we put it?  I think the most
logical place is in line-map.[ch], at least after we've
removed the old stuff.  Until then, it's a little trickier.

I could move the expanded_location typedef to line-map.h,
and add a linemap_expand_location method.  Then expand_location
becomes just synonym for linemap_expand_location, in the
USE_MAPPED_LOCATION case.

Ultimately, I'd want to add 'int column' to expanded_location,
but it complicates things as long as we have to support the
old machinery using struct location_s all over the place.

> > +#define LOCATION_FILE(FILELINE) __extension__ \
> > +#define LOCATION_LINE(FILELINE) __extension__ \
>
> Likewise.  I'm just as happy with these as lowercase out-of-line
> functions, always.  You'd have to prove to me that they're an
> actual bottleneck to want to do otherwise.

Maybe the "guts" of these can be moved to line-map.h.

I don't think either macro is used much, in the "new" model.

> > +#ifdef USE_MAPPED_LOCATION
> > +/* Say where in the code a source line starts, for symbol table's sake.
> > +   Operand:
> > +   4: unused if line number > 0, note-specific data otherwise.
> > +   5: line number if > 0, enum note_insn otherwise.
> > +   6: CODE_LABEL_NUMBER if line number == NOTE_INSN_DELETED_LABEL.  */
> > +#else
> > /* Say where in the code a source line starts, for symbol table's sake.
> >    Operand:
> >    4: filename, if line number > 0, note-specific data otherwise.
> > +   4: unused if line number > 0, note-specific data otherwise.
>
> One or both of these are wrong.

It should be:
#ifdef USE_MAPPED_LOCATION
   4: unused if line number > 0, note-specific data otherwise.
#else
   4: filename, if line number > 0, note-specific data otherwise.
#endif

It seems wasteful to use an extra slot for CODE_LABEL_NUMBER,
but I don't know enough to fix that.

> > +#ifdef USE_MAPPED_LOCATION
> > +  SET_EXPR_LOCATION (new_stmt, EXPR_LOCATION (orig_stmt));
> > +#else
> >   SET_EXPR_LOCUS (new_stmt, EXPR_LOCUS (orig_stmt));
> > +#endif
>
> Seems like this idiom could get mapped to
>
> #define EXPR_LOCUS(N) 		(&EXPR_LOCATION (N))
> #define SET_EXPR_LOCUS(N,L)	(EXPR_LOCATION (N) = *(L))
>
> or something similar to avoid the ifdeffery.

There'sa trickyness with NULL, but I'll try that.

> > +#ifdef USE_MAPPED_LOCATION
> > +  SET_EXPR_LOCATION (t, UNKNOWN_LOCATION);
> > +#else
> >   SET_EXPR_LOCUS (t, NULL);
> > +#endif
>
>
> Likewise invent a macro like CLEAR_EXPR_LOCATION.

There is a point of diminishing returns.  This construction
is only used twice, and the #ifdefery will go away once
the compiler has been converted.  But I can certainly add
a CLEAR_EXPR_LOCATION macro if you want.
--
	--Per Bothner
per@bothner.com   http://per.bothner.com/