LambdaLord / es-operating-system

Automatically exported from code.google.com/p/es-operating-system
Apache License 2.0
0 stars 0 forks source link

Code review request #28

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Purpose of code changes on this branch:
listen backlogs clearing up upon abortion

When reviewing my code changes, please focus on:
r1114

After the review, I'll merge this branch into:
/trunk

Original issue reported on code.google.com by ailvpen...@gmail.com on 1 Aug 2009 at 10:43

GoogleCodeExporter commented 9 years ago
Sorry to make a mistake.
When reviewing my code changes, please focus on:
r1142 and r1143

Original comment by ailvpen...@gmail.com on 1 Aug 2009 at 11:08

GoogleCodeExporter commented 9 years ago
I think that StreamReceiver::dumpAccepted() should be placed in 
testListenBKlogs.cpp
because it is just for testing. You may modify the function to take an instance 
of
StreamReceiver as an argument such as dumpAccepted(StreamReceiver* s).

Your change in stream.cpp looks good for me. As you commented, it seems that we
should cleanup conduit graph, but I am not sure about this is the right place 
to do
it. What do you think about this Shiki and Joao?

Original comment by Ishibash...@gmail.com on 3 Aug 2009 at 5:54

GoogleCodeExporter commented 9 years ago
Thank you Kenichi,

I agree with you to put StreamReceiver::dumpAccepted() in testListenBKlogs.cpp, 
It is
better.
But there is no member function to access StreamReceiver::accepted at the 
moment and
StreamReceiver::accepted is private. There may be some difficulty to dump 
accepted
queue outside StreamReceiver at the moment.

Original comment by ailvpen...@gmail.com on 5 Aug 2009 at 3:13

GoogleCodeExporter commented 9 years ago
I think you can introduce member function to get accepted queue from outside of 
the
StreamReceiver class. It may break encapsulation of the class, but it is 
somewhat
better than placing dumpAccepted() in the StreamReceiver class, in my opinion.

Original comment by Ishibash...@gmail.com on 5 Aug 2009 at 5:30

GoogleCodeExporter commented 9 years ago
When reviewing my code changes, please focus on:
r1142, r1143, r1175

Original comment by ailvpen...@gmail.com on 5 Aug 2009 at 8:08

GoogleCodeExporter commented 9 years ago
It look good for me, please commit the change to the trunk.

Original comment by Ishibash...@gmail.com on 8 Aug 2009 at 5:09

GoogleCodeExporter commented 9 years ago

Original comment by Ishibash...@gmail.com on 20 Aug 2009 at 3:44