[llvm-commits] [llvm] r75283 - in /llvm/trunk: include/llvm/CodeGen/AsmFormatter.h include/llvm/CodeGen/AsmStream.h include/llvm/Support/FormattedStream.h include/llvm/Support/raw_ostream.h lib/CodeGen/AsmStream.cpp lib/Support/FormattedStream.cpp lib/Support/raw_ostream.cpp

[Chris Lattner]

On Jul 10, 2009, at 3:59 PM, David Greene wrote:

> On Friday 10 July 2009 17:16, Chris Lattner wrote:
>
>> Thanks David, some more thoughts:
>
>>   /// raw_asm_fd_ostream - Formatted raw_fd_ostream to handle
>>   /// asm-specific constructs
>>
>> Please end comments with proper punctuation, this occurs many times.
>
> Eh?  This isn't a sentence.

Ok, well please fix it to be a sentence, that's a bug too :).

> The LLVM comment style is very non-uniform in
> this respect.  Sometimes periods are there, sometimes not.

If you notice cases where they are missing, please help improve this.

> I'd rather not take up time for simple things
> like this when there's no official policy in place.

Ok, I clarified the official policy:
llvm.org/docs/CodingStandards.html#scf_commenting

> Keep in mind that with every change some of us have to pass it by  
> internal
> reviewers and that takes time.

Ah, I used to have to go through that for a short period of time at  
Apple.  It didn't take long for me to convince the people who "vetted"  
contributions to make sure they didn't contain "valuable IP" that they  
should get themselves out of the process.  In a lot of ways, sending  
them a lot of trivial patches helps speed this along instead of  
batching stuff up.  They quickly stop looking at the patches and when/ 
if they start delaying responses unreasonably, it gives you a good  
reason to re-escalate the process issues.

In any case, having an unfortunate internal process isn't a reason for  
us to lower our standards, sorry. :(

>>
>>   myformatted_raw_ostream << "foo: ";
>>   myformatted_raw_ostream.PadToColumn(42);
>>   myformatted_raw_ostream << "bar";
>>
>> and it is semantically cleaner, because you're not inserting a  
>> column,
>> you're inserting a variable sized padding.
>
> When I did this originally I thought of it in the same way that  
> setw() and
> other such manipulators work.  It's a formatting thing and  
> traditionally
> that's done via manipulators.

Yep, understood.

>> +  /// AsmComment - An I/O manipulator to output an end-of-line  
>> comment
>> +  ///
>> +  class AsmComment : public Column {
>>
>> I still really don't see a reason for this to exist as an I/O
>> manipulator.  This seems like it should be a helper method on
>> AsmPrinter, not an I/O manipulator in a public header.
>
> Let me think about that a bit.
> ..
> I fiddled around and ended up getting rid of both manipulators.  The  
> resulting
> code's not quite as nice looking but as you say it's in a helper  
> function so
> that hides it a bit.

Awesome, thanks again for working on this David!

-Chris