porting pretty printing and astconsumers to raw_ostream patch

Hi!
Here is a patch what replaces std::ostream with llvm::raw_ostream.
This patch covers the AST library, but ignores Analysis lib. That will be ported later.

Cheers,
Csaba

raw_ostreamport.patch (25.5 KB)

Hi!
Here is a patch what replaces std::ostream with llvm::raw_ostream.
This patch covers the AST library, but ignores Analysis lib. That will be ported later.

Cheers,
Csaba

<raw_ostreamport.patch>_______________________________________________
cfe-dev mailing list
cfe-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

Hi Csaba,

This looks like a pretty good patch, although it introduces a “flushing” bug when using “raw_string_ostream” that appears over and over. Here are some specific comments:

===================================================================
— Driver/SerializationTest.cpp (revision 55284)
+++ Driver/SerializationTest.cpp (working copy)
@@ -64,12 +64,15 @@
TranslationUnit& TU) {
{
// Pretty-print the decls to a temp file.

  • std::ofstream DeclPP(FNameDeclPrint.c_str());
  • assert (DeclPP && “Could not open file for printing out decls.”);
  • std::string Err;
  • llvm::raw_fd_ostream DeclPP(FNameDeclPrint.c_str(), Err);
  • assert (Err.empty() && “Could not open file for printing out decls.”);
    llvm::OwningPtr FilePrinter(CreateASTPrinter(&DeclPP));

for (TranslationUnit::iterator I=TU.begin(), E=TU.end(); I!=E; ++I)
FilePrinter->HandleTopLevelDecl(*I);
+

  • DeclPP.flush();
    }

I don’t think the DeclPP.flush() is needed here, since DecpPP is stack allocated. Its destructor (which is called immediately afterwards) will flush the output. Having the explicit flush in this case just adds a little more clutter.

Here is the destructor for raw_fd_ostream:

raw_fd_ostream::~raw_fd_ostream() {
flush();
if (ShouldClose)
close(FD);
}

// Serialize the translation unit.
@@ -87,12 +90,15 @@

{
// Pretty-print the deserialized decls to a temp file.

  • std::ofstream DeclPP(FNameDeclPrint.c_str());
  • assert (DeclPP && “Could not open file for printing out decls.”);
  • std::string Err;
  • llvm::raw_fd_ostream DeclPP(FNameDeclPrint.c_str(), Err);
  • assert (Err.empty() && “Could not open file for printing out decls.”);
    llvm::OwningPtr FilePrinter(CreateASTPrinter(&DeclPP));

for (TranslationUnit::iterator I=NewTU->begin(), E=NewTU->end(); I!=E; ++I)
FilePrinter->HandleTopLevelDecl(*I);
+

  • DeclPP.flush();
    }

Same thing here. No need to explicitly call DeclPP.flush().

Index: Driver/RewriteObjC.cpp

— Driver/RewriteObjC.cpp (revision 55284)
+++ Driver/RewriteObjC.cpp (working copy)
@@ -25,9 +25,8 @@
#include “llvm/Support/MemoryBuffer.h”
#include “llvm/Support/CommandLine.h”
#include “llvm/Support/Streams.h”
+#include “llvm/Support/raw_ostream.h”
#include “llvm/System/Path.h”
-#include
-#include
using namespace clang;
using llvm::utostr;

@@ -453,20 +452,20 @@

// Create the output file.

  • std::ostream *OutFile;
  • llvm::raw_ostream *OutFile;
    if (OutFileName == “-”) {
  • OutFile = llvm::cout.stream();
  • OutFile = &llvm::outs();
    } else if (!OutFileName.empty()) {
  • OutFile = new std::ofstream(OutFileName.c_str(),
  • std::ios_base::binary|std::ios_base::out);
  • std::string Err;
  • OutFile = new llvm::raw_fd_ostream(OutFileName.c_str(), Err);
    } else if (InFileName == “-”) {
  • OutFile = llvm::cout.stream();
  • OutFile = &llvm::outs();
    } else {
    llvm::sys::Path Path(InFileName);
    Path.eraseSuffix();
    Path.appendSuffix(“cpp”);
  • OutFile = new std::ofstream(Path.toString().c_str(),
  • std::ios_base::binary|std::ios_base::out);
  • std::string Err;
  • OutFile = new llvm::raw_fd_ostream(Path.toString().c_str(), Err);
    }

RewriteInclude();
@@ -489,6 +488,7 @@
}
// Emit metadata.
*OutFile << ResultStr;

  • OutFile->flush();
    }

I don’t think you introduced this bug, but I think that there is a memory leak here. In the case where OutFile is not &llvm::outs(), the stream is never deallocated. One possible solution is to introduced an extra llvm::OwningPtr<> here. For example:

llvm::OwningPtrllvm::raw_ostream OwnedStream;
llvm::raw_ostream *OutFile;

else if (!OutFileName.empty()) {
OutFile = new llvm::raw_fd_ostream(…);
OwnedStream.reset(OutFile);
}

Thus, with just a little more code, the dynamically allocated raw_fd_ostream is deallocated at the end of the function.

Index: Driver/RewriteMacros.cpp

— Driver/RewriteMacros.cpp (revision 55284)
+++ Driver/RewriteMacros.cpp (working copy)
@@ -17,8 +17,8 @@
#include “clang/Lex/Preprocessor.h”
#include “clang/Basic/SourceManager.h”
#include “llvm/Support/Streams.h”
+#include “llvm/Support/raw_ostream.h”
#include “llvm/System/Path.h”
-#include
using namespace clang;

/// isSameToken - Return true if the two specified tokens start have the same
@@ -205,20 +205,20 @@
}

// Create the output file.

  • std::ostream *OutFile;
  • llvm::raw_ostream *OutFile;
    if (OutFileName == “-”) {
  • OutFile = llvm::cout.stream();
  • OutFile = &llvm::outs();
    } else if (!OutFileName.empty()) {
  • OutFile = new std::ofstream(OutFileName.c_str(),
  • std::ios_base::binary|std::ios_base::out);
  • std::string Err;
  • OutFile = new llvm::raw_fd_ostream(OutFileName.c_str(), Err);
    } else if (InFileName == “-”) {
  • OutFile = llvm::cout.stream();
  • OutFile = &llvm::outs();
    } else {
    llvm::sys::Path Path(InFileName);
    Path.eraseSuffix();
    Path.appendSuffix(“cpp”);
  • OutFile = new std::ofstream(Path.toString().c_str(),
  • std::ios_base::binary|std::ios_base::out);
  • std::string Err;
  • OutFile = new llvm::raw_fd_ostream(Path.toString().c_str(), Err);
    }

// Get the buffer corresponding to MainFileID. If we haven’t changed it, then
@@ -230,4 +230,5 @@
} else {
fprintf(stderr, “No changes\n”);
}

  • OutFile->flush();
    }

Same problem as before: possible memory leak of OutFile (this looks like a copy-paste).

Index: Driver/ASTConsumers.h

— Driver/ASTConsumers.h (revision 55284)
+++ Driver/ASTConsumers.h (working copy)
@@ -14,6 +14,7 @@
#ifndef DRIVER_ASTCONSUMERS_H
#define DRIVER_ASTCONSUMERS_H

+#include “llvm/Support/raw_ostream.h”
#include
#include

@@ -30,7 +31,7 @@
class Preprocessor;
class PreprocessorFactory;

-ASTConsumer CreateASTPrinter(std::ostream OS = NULL);
+ASTConsumer CreateASTPrinter(llvm::raw_ostream OS = NULL);

ASTConsumer *CreateASTDumper();

Index: lib/Rewrite/Rewriter.cpp

— lib/Rewrite/Rewriter.cpp (revision 55282)
+++ lib/Rewrite/Rewriter.cpp (working copy)
@@ -16,7 +16,7 @@
#include “clang/AST/Stmt.h”
#include “clang/Lex/Lexer.h”
#include “clang/Basic/SourceManager.h”
-#include
+#include “llvm/Support/raw_ostream.h”
using namespace clang;

void RewriteBuffer::RemoveText(unsigned OrigOffset, unsigned Size) {
@@ -166,9 +166,9 @@
return true;

// Get the new text.

  • std::ostringstream S;
  • std::string Str;
  • llvm::raw_string_ostream S(Str);
    To->printPretty(S);
  • const std::string &Str = S.str();

Looks like we’re missing a “flush” here before using the value of Str.

Here’s a thought. I added a “str()” method to raw_string_stream that does an implicit flush:

std::string& raw_string_ostream::str() {
flush();
return OS;
}

It’s in this commit: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080825/066409.html

This behavior more closely matches ostringstream, and I like it because it forces clients to think about retrieving the latest version of the string. Using str() removes a whole bunch of “flush” errors that your patch introduces (see the remainder of my comments to see how frequently this happens). Consequently, the current code that uses ostringstream can basically be left unmodified (except for swapping in raw_string_ostream), the necessary flushes get introduced, and we’re all happy.

ReplaceText(From->getLocStart(), Size, &Str[0], Str.size());
return false;
Index: lib/Rewrite/HTMLRewrite.cpp

— lib/Rewrite/HTMLRewrite.cpp (revision 55282)
+++ lib/Rewrite/HTMLRewrite.cpp (working copy)
@@ -20,7 +20,7 @@
#include “llvm/ADT/SmallString.h”
#include “llvm/ADT/OwningPtr.h”
#include “llvm/Support/MemoryBuffer.h”
-#include
+#include “llvm/Support/raw_ostream.h”
using namespace clang;

@@ -161,7 +161,8 @@
bool ReplaceTabs) {

unsigned len = s.size();

  • std::ostringstream os;
  • std::string Str;
  • llvm::raw_string_ostream os(Str);

for (unsigned i = 0 ; i < len; ++i) {

@@ -195,7 +196,7 @@
}
}

  • return os.str();
  • return Str;
    }

Definitely missing a flush. Use os.str() (which is nice, because the change on the last line is not needed).

static void AddLineNumber(RewriteBuffer &RB, unsigned LineNo,
@@ -272,7 +273,8 @@
SourceLocation StartLoc = SourceLocation::getFileLoc(FileID, 0);
SourceLocation EndLoc = SourceLocation::getFileLoc(FileID, FileEnd-FileStart);

  • std::ostringstream os;
  • std::string s;
  • llvm::raw_string_ostream os(s);
    os << “<!doctype html>\n” // Use HTML 5 doctype
    “\n\n”;

@@ -327,7 +329,7 @@
“\n\n”;

// Generate header

  • R.InsertStrBefore(StartLoc, os.str());
  • R.InsertStrBefore(StartLoc, s);

Missing flush. Just use os.str().

Index: lib/AST/CFG.cpp

static std::string getNodeLabel(const CFGBlock* Node, const CFG* Graph) {

#ifndef NDEBUG

  • std::ostringstream Out;
  • std::string OutStr;
  • llvm::raw_string_ostream Out(OutStr);
    print_block(Out,Graph, *Node, GraphHelper, false);
  • std::string OutStr = Out.str();

if (OutStr[0] == ‘\n’) OutStr.erase(OutStr.begin());

Another missing flush. Jut use os.str().

Index: lib/AST/StmtViz.cpp

— lib/AST/StmtViz.cpp (revision 55282)
+++ lib/AST/StmtViz.cpp (working copy)
@@ -33,14 +33,14 @@
static std::string getNodeLabel(const Stmt* Node, const Stmt* Graph) {

#ifndef NDEBUG

  • std::ostringstream Out;
  • std::string OutStr;
  • llvm::raw_string_ostream Out(OutStr);

if (Node)
Out << Node->getStmtClassName();
else
Out << “”;

  • std::string OutStr = Out.str();
    if (OutStr[0] == ‘\n’) OutStr.erase(OutStr.begin());

Another missing flush.

// Process string output to make it nicer…
Index: lib/AST/Type.cpp

— lib/AST/Type.cpp (revision 55282)
+++ lib/AST/Type.cpp (working copy)
@@ -870,9 +870,8 @@
S += ‘*’;

if (getSizeExpr()) {

  • std::ostringstream s;
  • llvm::raw_string_ostream s(S);
    getSizeExpr()->printPretty(s);
  • S += s.str();
    }
    S += ‘]’;

This is okay. The destructor for raw_string_stream does a flush to S before we hit the statement “S += ‘]’”;

@@ -897,9 +896,10 @@
void TypeOfExpr::getAsStringInternal(std::string &InnerString) const {
if (!InnerString.empty()) // Prefix the basic type, e.g. ‘typeof(e) X’.
InnerString = ’ ’ + InnerString;

  • std::ostringstream s;
  • std::string Str;
  • llvm::raw_string_ostream s(Str);
    getUnderlyingExpr()->printPretty(s);
  • InnerString = “typeof(” + s.str() + “)” + InnerString;
  • InnerString = “typeof(” + Str + “)” + InnerString;
    }

Another missing flush. Use s.str().

Index: lib/Driver/HTMLDiagnostics.cpp

— lib/Driver/HTMLDiagnostics.cpp (revision 55282)
+++ lib/Driver/HTMLDiagnostics.cpp (working copy)
@@ -23,9 +23,9 @@
#include “llvm/Support/Compiler.h”
#include “llvm/Support/MemoryBuffer.h”
#include “llvm/Support/Streams.h”
+#include “llvm/Support/raw_ostream.h”
#include “llvm/System/Path.h”
#include
-#include

using namespace clang;

@@ -220,7 +220,8 @@
// Add the name of the file as an

tag.

{

  • std::ostringstream os;
  • std::string s;
  • llvm::raw_string_ostream os(s);

os << “

Bug Summary

\n<table class=“simpletable”>\n”
“<td class=“rowname”>File:”
@@ -244,7 +245,7 @@

os << “\n

Annotated Source Code

\n”;

  • R.InsertStrBefore(SourceLocation::getFileLoc(FileID, 0), os.str());
  • R.InsertStrBefore(SourceLocation::getFileLoc(FileID, 0), s);
    }

Missing flush. Use os.str().

// Embed meta-data tags.
@@ -252,28 +253,32 @@
const std::string& BugDesc = D.getDescription();

if (!BugDesc.empty()) {

  • std::ostringstream os;
  • std::string s;
  • llvm::raw_string_ostream os(s);
    os << “\n\n”;
  • R.InsertStrBefore(SourceLocation::getFileLoc(FileID, 0), os.str());
  • R.InsertStrBefore(SourceLocation::getFileLoc(FileID, 0), s);
    }

Missing flush. Use os.str().

{

  • std::ostringstream os;
  • std::string s;
  • llvm::raw_string_ostream os(s);
    os << “\n\n”;
  • R.InsertStrBefore(SourceLocation::getFileLoc(FileID, 0), os.str());
  • R.InsertStrBefore(SourceLocation::getFileLoc(FileID, 0), s);
    }

Same thing.

{

  • std::ostringstream os;
  • std::string s;
  • llvm::raw_string_ostream os(s);
    os << “\n\n”;
  • R.InsertStrBefore(SourceLocation::getFileLoc(FileID, 0), os.str());
  • R.InsertStrBefore(SourceLocation::getFileLoc(FileID, 0), s);
    }

Same thing.

{

  • std::ostringstream os;
  • std::string s;
  • llvm::raw_string_ostream os(s);
    os << “\n\n”;
  • R.InsertStrBefore(SourceLocation::getFileLoc(FileID, 0), os.str());
  • R.InsertStrBefore(SourceLocation::getFileLoc(FileID, 0), s);
    }

Same thing.

// Add CSS, header, and footer.
@@ -365,7 +370,8 @@

// Create the html for the message.

  • std::ostringstream os;
  • std::string s;
  • llvm::raw_string_ostream os(s);

os << “\n<td class=“num”><td class=“line”>”
<< “<div id=”";
@@ -398,7 +404,7 @@
assert (false && “Unhandled hint.”);
}

  • R.InsertStrBefore(SourceLocation::getFileLoc(FileID, DisplayPos), os.str());
  • R.InsertStrBefore(SourceLocation::getFileLoc(FileID, DisplayPos), s);

Same thing.

For the uses of raw_string_ostream, I think the right thing to do is just remove your changes where your removed the call to “str()”. Other than fixing the two memory leaks I pointed out, the rest of the patch looks pretty good!

Hi!
I’ve done all changes what you had suggested.
Here is the second and improved patch what ports some clang code to use llvm::raw_ostream.

Cheers,
Csaba

raw_ostreamport.patch2 (23.7 KB)

Hi Csaba,

Sorry for the delay in replying. I was on vacation.

This patch looks great! Because you generated this patch 6 days ago, could you possible generate a new patch against top-of-tree? Feel free to just email it to me and I will apply it.

Cheers,
Ted