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 == ...
13 years, 11 months ago
(2011-04-20 15:12:13 UTC)
#2
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.
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 output
seems identical to the PPH_TRACE_TREE case.
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.
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.
13 years, 11 months ago
(2011-04-20 17:33:38 UTC)
#3
On 4/20/11, dnovillo@google.com <dnovillo@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
Issue 4433054: [pph] Namespaces, step 1. Trace formatting.
(Closed)
Created 13 years, 11 months ago by Lawrence Crowl
Modified 12 years, 10 months ago
Reviewers: Diego Novillo
Base URL: svn+ssh://gcc.gnu.org/svn/gcc/branches/pph/
Comments: 4