* Re: [pph] Stream merging information (issue 5090041)
@ 2011-09-21 20:47 dnovillo
2011-09-23 1:29 ` Lawrence Crowl
0 siblings, 1 reply; 3+ messages in thread
From: dnovillo @ 2011-09-21 20:47 UTC (permalink / raw)
To: crowl; +Cc: gcc-patches, reply
http://codereview.appspot.com/5090041/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):
http://codereview.appspot.com/5090041/diff/1/gcc/cp/pph-streamer-in.c#newcode2146
gcc/cp/pph-streamer-in.c:2146: pph_read_namespace_chain (pph_stream
*stream, tree enclosing_namespace)
2142 /* Read a chain of tree nodes from input block IB. DATA_IN
contains
2143 tables and descriptors for the file being read. */
2144
2145 tree
2146 pph_read_namespace_chain (pph_stream *stream, tree
enclosing_namespace)
ENCLOSING_NAMESPACE needs documenting. Would it be better to have the
original pph_read_chain() get this argument? Not crazy about this
duplication of code.
Same comment applies to the other two routines that the patch
duplicates.
http://codereview.appspot.com/5090041/
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pph] Stream merging information (issue 5090041)
2011-09-21 20:47 [pph] Stream merging information (issue 5090041) dnovillo
@ 2011-09-23 1:29 ` Lawrence Crowl
0 siblings, 0 replies; 3+ messages in thread
From: Lawrence Crowl @ 2011-09-23 1:29 UTC (permalink / raw)
To: crowl, dnovillo, gcc-patches, reply
On 9/21/11, dnovillo@google.com <dnovillo@google.com> wrote:
> http://codereview.appspot.com/5090041/diff/1/gcc/cp/pph-streamer-in.c
> File gcc/cp/pph-streamer-in.c (right):
>
> http://codereview.appspot.com/5090041/diff/1/gcc/cp/pph-streamer-in.c#newcode2146
> gcc/cp/pph-streamer-in.c:2146: pph_read_namespace_chain (pph_stream
> *stream, tree enclosing_namespace)
> 2142 /* Read a chain of tree nodes from input block IB. DATA_IN
> contains
> 2143 tables and descriptors for the file being read. */
> 2144
> 2145 tree
> 2146 pph_read_namespace_chain (pph_stream *stream, tree
> enclosing_namespace)
>
> ENCLOSING_NAMESPACE needs documenting.
Copy/paste/edit failure.
> Would it be better to have the original pph_read_chain() get
> this argument? Not crazy about this duplication of code.
There is no origional pph_read_chain. The pph_in_chain uses
streamer_read_chain. I didn't think altering that API was the
right thing to do for a pph-specific feature.
> Same comment applies to the other two routines that the patch
> duplicates.
In the end, I decided that the duplication was likely to be worth it
to avoid all the pointer passing and checking. Most trees streamed
will not be direct children of namespaces.
--
Lawrence Crowl
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pph] Stream merging information (issue 5090041)
@ 2011-09-26 20:11 dnovillo
0 siblings, 0 replies; 3+ messages in thread
From: dnovillo @ 2011-09-26 20:11 UTC (permalink / raw)
To: crowl; +Cc: gcc-patches, reply
http://codereview.appspot.com/5090041/diff/1/gcc/cp/pph-streamer-out.c
File gcc/cp/pph-streamer-out.c (right):
http://codereview.appspot.com/5090041/diff/1/gcc/cp/pph-streamer-out.c#newcode1924
gcc/cp/pph-streamer-out.c:1924: tree enclosing_namespace )
1922 void
1923 pph_write_namespace_tree (pph_stream *stream, tree expr,
1924 tree enclosing_namespace )
Just found this while changing other code. This needs a comment. no
space before ')'.
http://codereview.appspot.com/5090041/
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-09-26 18:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-21 20:47 [pph] Stream merging information (issue 5090041) dnovillo
2011-09-23 1:29 ` Lawrence Crowl
2011-09-26 20:11 dnovillo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).