[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

[David Greene]

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.  The LLVM comment style is very non-uniform in
this respect.  Sometimes periods are there, sometimes not.  I'll put periods 
in (I _guess_ that's what you want though it doesn't make sense to me) but 
it would be good to have some official guidance.  The coding standards 
document doesn't mention it AFAIK.

Keep in mind that with every change some of us have to pass it by internal
reviewers and that takes time.  I'd rather not take up time for simple things
like this when there's no official policy in place.

>      /// Column - The current output column of the stream
>      ///
>      unsigned Column;
>
> Please mention that this is 0 based, not 1 based.

Ok, that's a good idea.

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

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

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.

You're absolutely right about the typing problem, but that's the price we
pay for coupling stream buffers to stream objects.

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

Yep. good point.

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

                                 -Dave