On Dec 15, 2009, at 6:33 PM, David Greene wrote:> On Tuesday 15 December 2009 20:11, Chris Lattner wrote: > >> Please send complete and minimal patches to avoid wasting reviewer's >> time, > > What do you want, complete or minimal? I don't want to pre-send 50 > or so patches (one per source file) that simply change errs() to dbgs(). > Thus I thought I would send a patch representative of what will happen. > > What, specifically, do you want reviewed before I start checking in? > I'll be happy to prepare a patch that shows just that.I want a patch that does one thing (e.g. implements dbgs(), optionally switches clients to use it) that is complete enough that I can apply it to the tree and test it if I so choose. It should not have unrelated stuff in it (like other unrelated work). -Chris
On Tuesday 15 December 2009 23:02, Chris Lattner wrote:> On Dec 15, 2009, at 6:33 PM, David Greene wrote: > > On Tuesday 15 December 2009 20:11, Chris Lattner wrote: > >> Please send complete and minimal patches to avoid wasting reviewer's > >> time, > > > > What do you want, complete or minimal? I don't want to pre-send 50 > > or so patches (one per source file) that simply change errs() to dbgs(). > > Thus I thought I would send a patch representative of what will happen. > > > > What, specifically, do you want reviewed before I start checking in? > > I'll be happy to prepare a patch that shows just that. > > I want a patch that does one thing (e.g. implements dbgs(),Ok, let's start with that. Here's the patch to add the circular raw_ostream. Do you want another patch that modifies Debug.h and then one more patch that shows client use? -Dave -------------- next part -------------- A non-text attachment was scrubbed... Name: dbgs.patch Type: text/x-diff Size: 6213 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20091216/2d420987/attachment.patch>
On Dec 16, 2009, at 8:12 AM, David Greene wrote:>>> >>> What, specifically, do you want reviewed before I start checking in? >>> I'll be happy to prepare a patch that shows just that. >> >> I want a patch that does one thing (e.g. implements dbgs(), > > Ok, let's start with that. Here's the patch to add the circular raw_ostream.This idea of the patch makes sense to me, but here are a few questions: + int BufferSize; + std::vector<char> BufferArray; + bool DelayOutput; + std::vector<char>::iterator Cur; Please doxygenify these. Instead of using a std::vector for BufferArray, please just new[] an array since it is fixed size. Why is BufferSize needed with the vector? Isn't it always the size of BufferArray? Also, why is it signed? What does DelayOutput do? Even reading the code, I don't really understand what it is doing. + circular_raw_ostream(raw_ostream &Stream, int BufferSize = -1, + bool Delete = false) + : raw_ostream(/*unbuffered*/true), TheStream(0), DeleteStream(false), + BufferArray(BufferSize < 0 ? 8192 : BufferSize), Please make BufferSize an 'unsigned' and default it to 8192. Please use PRESERVE_STREAM instead of 'false'.> Do you want another patch that modifies Debug.h and then one more patch that > shows client use?Sure, you don't need to convert everything over, but once the general approach looks fine you can do the rest without review. -Chris -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20091216/4fdc4387/attachment.html>