On 7/26/11, Gabriel Charette <gch...@google.com> wrote:
>> +/* Load a tinst_level list. */
>> +
>> +static struct tinst_level *
>> +pph_in_tinst_level (pph_stream *stream)
>> +{
>> + struct tinst_level *last = NULL;
>> + unsigned count = pph_in_uint (stream);
>> + /* FIXME pph: This leaves the list in reverse order. Issue? */
>> + for (; count > 0; --count)
>> + {
>> + struct tinst_level *cur = ggc_alloc_tinst_level ();
>> + cur->next = last;
>
> - cur->next = last;
> + if(last)
> + last->next = cur;
>
>> + cur->decl = pph_in_tree (stream);
>> + cur->locus = pph_in_location (stream);
>> + cur->errors = pph_in_uint (stream);
>> + cur->in_system_header_p = pph_in_uint (stream);
>> + last = cur;
>> + }
>
> if (last)
> last->next = NULL;
>
>> + return last;
>> +}
>> +
>
> If you do the changes above, the list won't be in reverse order.
That code loses the pointer to the head of the list. However, I
take your point and will avoid the comment by doing the forward list.
> Also if you do it as I did in pph_in_cxx_binding, using the cache
> markers, you do need to output count, you can simply use the cache
> mechanism which will know how to output/input NULL (I used that in
> pph_in/out_cxx_binding).
I think I like the count better in this instance.
>> +/* Load and merge a spec_entry TABLE from STREAM. */
>> +
>> +static void
>> +pph_in_spec_entry_htab (pph_stream *stream, htab_t *table)
>> +{
>> + spec_entry **slot = NULL;
>
> This variable is shadowed by....
>
>> + unsigned count = pph_in_uint (stream);
>> + if (flag_pph_debug >= 2)
>> + fprintf (stderr, "loading %d spec_entries\n", count );
>> + for (; count > 0; --count)
>> + {
>> + hashval_t hash;
>> + spec_entry **slot;
>
> ...this one??
>
>> + struct spec_entry *se = ggc_alloc_spec_entry ();
>> + se->tmpl = pph_in_tree (stream);
>> + se->args = pph_in_tree (stream);
>> + se->spec = pph_in_tree (stream);
>> + hash = hash_specialization (se);
>> + slot = htab_find_slot_with_hash (*table, se, hash, INSERT);
>> + *slot = se;
>> + }
>> +}
I must have removed that earlier.
>> +/* Read and return a location_t from STREAM. */
>> +static inline location_t
>> +pph_in_location (pph_stream *stream)
>> +{
>> + location_t loc = lto_input_location (stream->ib, stream->data_in);
>> + if (flag_pph_tracer >= 4)
>> + pph_trace_location (stream, loc);
>> + return loc;
>> +}
>> +
>
> I was actually going to create pph_in/out_location. I
> will need to make them a streamer_hook so that all calls to
> lto_input/output_location actually are redirected to those when
> lto wants to in/out a source_location. Of course it would no
> longer call lto_input/output_location, rather it would output
> the source_location directly, and apply the calculated offset
> (discussed in "Linemap and pph") on the way in. It's great that
> you implemented a tracer for this as it will be easy for me to
> debug my implementation of this, thanks!
>
> Is there anything you implementation depends on I should be careful
> about, from what I see, it doesn't look like it (as long as the
> source_location you get back from pph_in_location are correct).
No particular dependences. The templates needed locations but
weren't in trees. They aren't doing anything special with them
that I saw.
>> +extern void lto_output_location (struct output_block *ob, location_t loc);
>
> lto_input/ouput will again no longer need to be extern when I make
> my patch for pph to handle those differently, I will make sure
> (remind me if I don't) to remove this change.
Okay.
--
Lawrence Crowl
