[cfe-dev] porting pretty printing and astconsumers to raw_ostream patch
Ted Kremenek
kremenek at apple.com
Tue Aug 26 09:40:36 PDT 2008
On Aug 24, 2008, at 8:08 AM, Csaba Hruska wrote:
> 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 at 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<ASTConsumer>
> 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<ASTConsumer>
> 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 <sstream>
> -#include <fstream>
> 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::OwningPtr<llvm::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 <fstream>
> 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 <string>
> #include <iosfwd>
>
> @@ -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 <sstream>
> +#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 <sstream>
> +#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
> "<html>\n<head>\n";
>
> @@ -327,7 +329,7 @@
> "</style>\n</head>\n<body>";
>
> // 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 << "<NULL>";
>
> - 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 <fstream>
> -#include <sstream>
>
> using namespace clang;
>
> @@ -220,7 +220,8 @@
> // Add the name of the file as an <h1> tag.
>
> {
> - std::ostringstream os;
> + std::string s;
> + llvm::raw_string_ostream os(s);
>
> os << "<h3>Bug Summary</h3>\n<table class=\"simpletable\">\n"
> "<tr><td class=\"rowname\">File:</td><td>"
> @@ -244,7 +245,7 @@
>
> os << "</table>\n<h3>Annotated Source Code</h3>\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<!-- BUGDESC " << BugDesc << " -->\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<!-- BUGFILE " << DirName << Entry->getName() << " -->
> \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<!-- BUGLINE " << D.back()-
> >getLocation().getLogicalLineNumber()
> << " -->\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<!-- BUGPATHLENGTH " << D.size() << " -->\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<tr><td class=\"num\"></td><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!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080826/5cfaf2b8/attachment.html>
More information about the cfe-dev
mailing list