[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 2:14 PM, David Greene wrote:
> Author: greened
> Date: Fri Jul 10 16:14:44 2009
> New Revision: 75283
>
> URL: http://llvm.org/viewvc/llvm-project?rev=75283&view=rev
> Log:
>
> Make changes suggested by Chris and eliminate newly-added raw_ostream
> hooks as they're no longer needed.
>
> The major change with this patch is to make formatted_raw_ostream  
> usable
> by any client of raw_ostream.

Thanks David, some more thoughts:

//===-- llvm/CodeGen/FormattedStream.h - Formatted streams ------*- C+ 
+ -*-===//
...
#ifndef LLVM_CODEGEN_ASMSTREAM_H
#define LLVM_CODEGEN_ASMSTREAM_H

Please update the ifdefs.

   /// raw_asm_fd_ostream - Formatted raw_fd_ostream to handle
   /// asm-specific constructs

Please end comments with proper punctuation, this occurs many times.

     /// Column - The current output column of the stream
     ///
     unsigned Column;

Please mention that this is 0 based, not 1 based.



   /// operator<< - Support coulmn-setting in formatted streams.

typo "column".


   inline formatted_raw_ostream &operator<<(formatted_raw_ostream &Out,
                                            const Column &Func) {
     return(Func(Out));
   }

Is this really going to work?  If I do:


   myformatted_raw_ostream << "foo: " << Column(42) << "bar";

Then the LHS of the << Column is: (myformatted_raw_ostream << "foo: ")  
which is not a formatted_raw_ostream.  I don't see a great reason to  
keep insertion-style syntax for this.  It's not *that* horrible to  
have to do:


   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.



//===-- llvm/CodeGen/AsmStream.cpp - AsmStream Framework --------*- C+ 
+ -*-===//

Please update.

// This file was developed by the LLVM research group and is  
distributed under
// the University of Illinois Open Source License. See LICENSE.TXT for  
details.

Please update.

//
// 
= 
= 
=---------------------------------------------------------------------- 
===//
//
// This file contains instantiations of "standard" AsmOStreams.

Please update.


#include "llvm/Support/FormattedStream.h"
namespace llvm {


Please use "using namespace llvm;" in .cpp files to avoid nesting the  
whole file.

formatted_raw_ostream::ComputeColumn is over-complicated.  Instead of  
scanning backwards from the end to find the \n, just do something like:

   for (; Size; ++Ptr, --Size) {
     if (*Ptr == '\n' || *Ptr == '\r')
       Column = 0;
     else if (*Ptr == '\t')
       Column += (8 - (Column & 0x7)) & 0x7;
     else
       ++Column;
   }

That's enough, isn't it?  Please handle \r like \n in any case.

+  /// 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.

-Chris