[llvm-commits] [llvm] r75202 - in /llvm/trunk: include/llvm/CodeGen/AsmStream.h lib/CodeGen/AsmStream.cpp

[Chris Lattner]

On Jul 9, 2009, at 4:56 PM, David Greene wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=75202&view=rev
> Log:
>
> Redesign this to avoid standard stream classes.  This stream class
> provides pretty -printing of comments and other such things in asm
> files.

This looks really cool.  We should discuss how the .s printer will use  
it though, because it is very performance sensitive.  Making the .ll  
printer use it should be no problem.


> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/include/llvm/CodeGen/AsmStream.h (added)
> +++ llvm/trunk/include/llvm/CodeGen/AsmStream.h Thu Jul  9 18:56:35  
> 2009
> @@ -0,0 +1,179 @@
> +//===-- llvm/CodeGen/AsmStream.h - AsmStream Framework --------*- C+ 
> + -*-===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// 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.

This is the old header, please update.

> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +//
> +// This file contains raw_ostream implementations for ASM printers to
> +// do things like pretty-print comments.
> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +
> +#ifndef LLVM_CODEGEN_ASMSTREAM_H
> +#define LLVM_CODEGEN_ASMSTREAM_H
> +
> +#include "llvm/Target/TargetAsmInfo.h"
> +#include <llvm/Support/raw_ostream.h>
> +
> +namespace llvm
> +{
> +  /// raw_asm_fd_ostream - Formatted raw_fd_ostream to handle
> +  /// asm-specific constructs
> +  ///
> +  class raw_asm_fd_ostream : public raw_fd_ostream {

This seems like it is a more general than just for assembly printing.   
I would suggest naming this something like formatted_raw_ostream or  
something like that, and then have "asm comment" stuff be handled  
orthogonally to it.  It seems weird for stuff that is this stream- 
specific to know about TargetAsmInfo, for example.  This also allows  
the file to live in libsupport.

> +  private:
> +    bool formatted;
> +    int column;

Please add doxygen comments for instance variables and capitalize  
their first character.

> +
> +  protected:
> +    void ComputeColumn(void) {

(void) isn't needed in C++, also other places in the file.  Please add  
doxygen comment.

> +      if (formatted) {
> +        // Keep track of the current column by scanning the string  
> for
> +        // special characters

Comments should end with a period when they are sentences.  Also later  
in the file.


More generally, I'm concerned that ComputeColumn isn't the right way  
to go.  Why not have your flush_impl method just keep track of the  
current column as the code flies by?  A "setColumn" could just then do  
a flush (updating the counter) and then act on it.  This can also be  
defined out of line in the .cpp file.


> +    /// raw_asm_fd_ostream - Open the specified file for writing. If
> +    /// an error occurs, information about the error is put into
> +    /// ErrorInfo, and the stream should be immediately destroyed;  
> the
> +    /// string will be empty if no error occurred.
> +    ///
> +    /// \param Filename - The file to open. If this is "-" then the
> +    /// stream will use stdout instead.
> +    /// \param Binary - The file should be opened in binary mode on
> +    /// platforms that support this distinction.
> +    raw_asm_fd_ostream(const char *Filename, bool Binary,  
> std::string &ErrorInfo)
> +        : raw_fd_ostream(Filename, Binary, ErrorInfo),
> +            formatted(!Binary), column(0) {}

This should not inherit from raw_fd_ostream, it should inherit from  
raw_ostream and take another raw_ostream by-ref to send things through.

> +
> +    /// raw_asm_fd_ostream ctor - FD is the file descriptor that this
> +    /// writes to.  If ShouldClose is true, this closes the file when
> +    /// the stream is destroyed.
> +    raw_asm_fd_ostream(int fd, bool shouldClose,
> +                       bool unbuffered=false)

Unbuffered output should not be an option here.  There is no need to  
keep the interface of raw_fd_ostream.

> +    /// SetColumn - Align the output to some column number
> +    ///
> +    void setColumn(int newcol, int minpad = 0) {

The name in the comment and the name of the method differ.  However,  
please rename this to "JumpToColumn" or "PadToColumn" something to  
indicate that this actually emits stuff.
>
> +  /// Column - An I/O manipulator to advance the output to a  
> certain column
> +  ///
> +  class Column {
> +  private:
> +    int column;

column should be unsigned, documented, and capitalized.

> +
> +  public:
> +    explicit Column(int c)
> +        : column(c) {}
> +
> +    raw_asm_fd_ostream &operator()(raw_asm_fd_ostream &out) const {
> +      // Make at least one space before the comment
> +      out.setColumn(column, 1);
> +      return(out);
> +    }
> +  };

This allows things like:
   O << "foo" << Column(42) << "bar";

?  Cute.

> +  inline raw_asm_fd_ostream &operator<<(raw_asm_fd_ostream &out,  
> const Column &column)
> +  {
> +    return(column(out));
> +  }

Can this be a generic templated method for raw_ostream to handle any  
manipulator?

> +
> +  /// Comment - An I/O manipulator to output an end-of-line comment
> +  ///
> +  class Comment
> +      : public Column {

This seems to have dubious value, but in any case should definitely  
not be in the main formatted ostream header.

> +  /// Asm stream equivalent for std streams
> +  ///
> +
> +  /// WARNING: Do NOT use these streams in the constructors of global
> +  /// objects.  There is no mechanism to ensure they are initialized
> +  /// before other global objects.
> +  ///
> +  extern raw_asm_fd_ostream asmouts;
> +  extern raw_asm_fd_ostream asmerrs;

Please remove these.

> +namespace llvm {
> +  raw_asm_fd_ostream asmouts(STDOUT_FILENO, false);
> +  raw_asm_fd_ostream asmerrs(STDERR_FILENO, false);
> +}

Please don't do this.  It is a) not portable at all, b) uses static  
ctors, which we're trying to eliminate, and c) not needed.  The  
formatted ostreams should just chain to another ostream, and clients  
can use outs() or something as desired.

-Chris