src/mem/packet.hh (Diff revision 1)

Review board seems to be flagging some whitespace you have at the end of this line. The idea of this function seems fine but its name is a little awkward. How about hasData()?

I'm not sure this change does what you think it does. those flags only mean that the packet has data storage allocated, it doesn't say anything about the validity of that data. I can't think of a good reason why someone external would need to know that the packet has data allocated. If that is what you're going for, then it should have a name that makes it clear that this just means storage, nothing about the protocol and a good doxygen comment.

Brad, are you sure correct file go uploaded? I see html tags in the file.

Looks reasonable to me.

src/mem/packet.hh (Diff revision 3)

should be a const function.

Sorry I haven't plowed through all the other review requests, but where is this used in Ruby?  What happens if the packet doesn't have data?  Would it make sense to combine the patches that use this function with this patch?  (My thought is probably so.)

src/mem/packet.hh (Diff revision 3)

You need spaces around |.

1. Brad Beckmann Jan. 10, 2011, 7:50 a.m. (Jan. 10, 2011, 7:50 a.m.)

   I can add a space around |, but hat is inconsistent with the rest of the file.  

2. Gabe Black Jan. 10, 2011, 8:59 a.m. (Jan. 10, 2011, 8:59 a.m.)

   Ah, yeah, I see that now. I don't think that's correct style generally speaking, but you're probably right to stay consistent with the rest of the file.

src/mem/packet.hh (Diff revision 3)

I'd still prefer hasDataAllocated() or something 

src/mem/ruby/system/RubyPort.cc (Diff revision 4)

I suggest changing Packet::getPtr() to take an optional flag like this:
 getPtr(bool null_ok = false)
 {
   assert(null_ok ||
     flags.isSet(STATIC_DATA|DYNAMIC_DATA));
   return (T*)data;
 }
(assuming that data really is null when those flags are clear), then changing this section to just call getPtr<uint8_t>(true) (you might not even need the ruby_data temp anymore, I'm not sure), and you won't even need the method that you can't find a good name for :-)

Obviates the need for a different patch I had in my own queue, and more general.  Thanks Brad.