On 4/20/11, dnovi...@google.com <dnovi...@google.com> wrote: > http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.c > File gcc/cp/pph-streamer.c (right): > > http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.c#newcode144 > gcc/cp/pph-streamer.c:144: return; > + if ((type == PPH_TRACE_TREE || type == PPH_TRACE_CHAIN) > + && !data && flag_pph_tracer <= 3) > + return; > > Line up the predicates vertically.
Can you be more specific? > > http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.c#newcode172 > gcc/cp/pph-streamer.c:172: fprintf (pph_logfile, ", code=%s", > tree_code_name[TREE_CODE (t)]); > case PPH_TRACE_REF: > + { > + const_tree t = (const_tree) data; > + if (t) > + { > + print_generic_expr (pph_logfile, CONST_CAST (union tree_node *, > t), > + 0); > + fprintf (pph_logfile, ", code=%s", tree_code_name[TREE_CODE (t)]); > > > But how are we going to tell if this is a REF instead of a tree? The type_s array is indexed by PPH_TRACE_REF. > The output seems identical to the PPH_TRACE_TREE case. Well, the case in those branches is identical. The splitting was a bit preemptive, as I was planning to see what changes I needed after seeing what items were refs. None actually were refs, so the distinction isn't there. > http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.h > File gcc/cp/pph-streamer.h (right): > > http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.h#newcode149 > gcc/cp/pph-streamer.h:149: } > pph_output_tree_lst (pph_stream *stream, tree t, bool ref_p) > +{ > + if (flag_pph_tracer >= 2) > + pph_stream_trace_tree (stream, t, ref_p); > + lto_output_tree (stream->ob, t, ref_p); > +} > > I don't really like all this code duplication. Wouldn't it be better if > instead of having pph_output_tree_aux and pph_output_tree_lst, we added > another argument to pph_output_tree? The argument would be an enum and > we could have a default 'DONT_CARE' value. I'm not sure that would save much code. It would induce some runtime overhead (unless the compiler specialized routines). It would also change the callbacks. > http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.h#newcode298 > gcc/cp/pph-streamer.h:298: pph_stream_trace_tree (stream, t, false); /* > FIXME pph: always false? */ > @@ -285,7 +295,7 @@ pph_input_tree (pph_stream *stream) > { > tree t = lto_input_tree (stream->ib, stream->data_in); > if (flag_pph_tracer >= 4) > - pph_stream_trace_tree (stream, t); > + pph_stream_trace_tree (stream, t, false); /* FIXME pph: always > false? > > Yes, on input we can't tell if we read a reference or a real tree. We > could, but not at this level. That's inside the actual LTO streaming > code. It would be nice to have an indication, but it is not something I want to do now. > > http://codereview.appspot.com/4433054/ -- Lawrence Crowl
