<div class="gmail_quote">On 14 April 2010 07:44, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div style="word-wrap: break-word;"><div class="im"><br><div><div>On Apr 9, 2010, at 7:24 PM, Nick Lewycky wrote:</div><br><blockquote type="cite">Currently the fixit system runs in one of two modes, either fix the particular problems specified through the -fixit-at parameter, or fix only the problems found in the main file. As a bonus bug, in the fix-all mode, clang's diagnostic output will claim that it's applied changes to files that it isn't actually touching.<div>
<br></div><div>This patch makes fixits apply to all files when no -fixit-at or -o flag is specified. For compatibility with our tests, clang -fixit -o foo will still only output the main file (but see FIXME in patch). This also exposes the fileid's out the FixItRewriter so that users can write their own frontend action which chooses file names other than the default <a href="http://foo.fixit.cc/" target="_blank">foo.fixit.cc</a>.</div>
</blockquote></div><div><br></div></div><div><div>--- /tmp/g4-68652/cache/depot/google3/third_party/llvm/trunk/tools/clang/lib/Frontend/FixItRewriter.cpp#4<span style="white-space: pre;"> </span>2010-04-08 23:10:18.000000000 -0700</div>
<div>+++ /home/nlewycky/llvm-ftw1/google3/third_party/llvm/trunk/tools/clang/lib/Frontend/FixItRewriter.cpp<span style="white-space: pre;"> </span>2010-04-09 17:17:26.741262000 -0700</div><div>@@ -14,6 +14,8 @@</div><div>
//===----------------------------------------------------------------------===//</div><div> </div><div> #include "clang/Frontend/FixItRewriter.h"</div><div>+#include "clang/Basic/FileManager.h"</div>
<div>
+#include "clang/Basic/SourceLocation.h"</div><div> #include "clang/Basic/SourceManager.h"</div><div> #include "clang/Frontend/FrontendDiagnostic.h"</div><div> #include "llvm/Support/raw_ostream.h"</div>
<div>@@ -34,47 +36,41 @@</div><div> Diags.setClient(Client);</div><div> }</div><div> </div><div>-bool FixItRewriter::WriteFixedFile(const std::string &InFileName,</div><div>- const std::string &OutFileName) {</div>
<div>+bool FixItRewriter::WriteFixedFile(FileID ID, llvm::raw_ostream *OS) {</div><div>+ const RewriteBuffer *RewriteBuf = Rewrite.getRewriteBufferFor(ID);</div><div>+ if (!RewriteBuf) return true;</div><div>+ *OS << std::string(RewriteBuf->begin(), RewriteBuf->end());</div>
<div>+ OS->flush();</div><div>+ return false;</div><div>+}</div><div><br></div><div>Please use OS->write() instead of forming a temporary string. </div></div></div></blockquote><div><br>No can do. It's not backed by contiguous storage being a btree-string and there's no existing API to print these things without an std::string temporary. In a future patch I'll add a print(raw_ostream&) method to RewriteBuffer or one of its base classes, with the inefficient implementation and a FIXME to make it more awesome.<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div style="word-wrap: break-word;"><div><div>Also, shouldn't the raw_ostream be a reference parameter rather than a pointer parameter, since it can't be NULL anyway?</div>
</div></div></blockquote><div><br>Done.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div style="word-wrap: break-word;"><div>
<div>
</div><div><div>+</div><div>+bool FixItRewriter::WriteFixedFiles() {</div><div> if (NumFailures > 0) {</div><div> Diag(FullSourceLoc(), diag::warn_fixit_no_changes);</div><div> return true;</div><div> }</div>
<div> </div><div>- llvm::OwningPtr<llvm::raw_ostream> OwnedStream;</div><div>- llvm::raw_ostream *OutFile;</div><div>- if (!OutFileName.empty()) {</div><div>- std::string Err;</div><div>- OutFile = new llvm::raw_fd_ostream(OutFileName.c_str(), Err,</div>
<div>- llvm::raw_fd_ostream::F_Binary);</div><div>- OwnedStream.reset(OutFile);</div><div>- } else if (InFileName == "-") {</div><div>- OutFile = &llvm::outs();</div>
<div>- } else {</div><div>- llvm::sys::Path Path(InFileName);</div><div>+ for (iterator I = buffer_begin(), E = buffer_end(); I != E; ++I) {</div><div>+ const FileEntry *Entry = Rewrite.getSourceMgr().getFileEntryForID(I->first);</div>
<div>+ llvm::sys::Path Path(Entry->getName());</div><div> std::string Suffix = Path.getSuffix();</div><div> Path.eraseSuffix();</div><div> Path.appendSuffix("fixit." + Suffix);</div><div> std::string Err;</div>
<div>- OutFile = new llvm::raw_fd_ostream(Path.c_str(), Err,</div><div>- llvm::raw_fd_ostream::F_Binary);</div><div>- OwnedStream.reset(OutFile);</div><div>- }</div><div>+ llvm::raw_fd_ostream *OS = new llvm::raw_fd_ostream(Path.c_str(), Err,</div>
<div>+ llvm::raw_fd_ostream::F_Binary);</div></div></div><div><br></div>The raw_fd_ostream could/should be stack-allocated.</div></blockquote><div><br>Done.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div style="word-wrap: break-word;"><div></div><div>Okay, so this will overwriter the files in place. We should be prepared for the write to fail, e.g., if the source file is read-only (and emit a reasonable diagnostic when it does).<br>
<div><div><br></div><div>- FileID MainFileID = Rewrite.getSourceMgr().getMainFileID();</div><div>- if (const RewriteBuffer *RewriteBuf =</div><div>- Rewrite.getRewriteBufferFor(MainFileID)) {</div><div>- *OutFile << std::string(RewriteBuf->begin(), RewriteBuf->end());</div>
<div>- } else {</div><div>- Diag(FullSourceLoc(), diag::note_fixit_main_file_unchanged);</div><div>+ RewriteBuffer &RewriteBuf = I->second;</div><div>+ *OS << std::string(RewriteBuf.begin(), RewriteBuf.end());</div>
<div>+ OS->flush();</div><div>+ delete OS;</div><div><br></div><div>Same OS.write() comment as above.</div><div><br></div><div><div>==== //depot/google3/third_party/llvm/trunk/tools/clang/lib/Frontend/FrontendActions.cpp#7 - /home/nlewycky/llvm-ftw1/google3/third_party/llvm/trunk/tools/clang/lib/Frontend/FrontendActions.cpp ====</div>
<div>--- /tmp/g4-68652/cache/depot/google3/third_party/llvm/trunk/tools/clang/lib/Frontend/FrontendActions.cpp#7<span style="white-space: pre;"> </span>2010-04-08 21:49:14.000000000 -0700</div><div>+++ /home/nlewycky/llvm-ftw1/google3/third_party/llvm/trunk/tools/clang/lib/Frontend/FrontendActions.cpp<span style="white-space: pre;"> </span>2010-04-09 15:59:12.323217000 -0700</div>
<div>@@ -139,6 +139,23 @@</div><div> FixItRewrite.addFixItLocation(Requested);</div><div> }</div><div> </div><div>+ const std::string &OutputFile = CI.getFrontendOpts().OutputFile;</div><div>+ if (Locs.empty() && !OutputFile.empty()) {</div>
<div>+ // FIXME: we will issue "FIX-IT applied suggested code changes" for every</div><div>+ // input, but only the main file will actually be rewritten.</div><div>+ const std::vector<std::pair<FrontendOptions::InputKind, std::string> > &Inputs =</div>
<div>+ CI.getFrontendOpts().Inputs;</div><div>+ for (unsigned i = 0, e = Inputs.size(); i != e; ++i) {</div><div>+ const FileEntry *File = CI.getFileManager().getFile(Inputs[i].second);</div><div>+ assert(File && "Input file not found in FileManager");</div>
<div>+ RequestedSourceLocation Requested;</div><div>+ Requested.File = File;</div><div>+ Requested.Line = 0;</div><div>+ Requested.Column = 0;</div><div>+ FixItRewrite.addFixItLocation(Requested);</div>
<div>+ }</div><div>+ }</div><div>+</div><div> return true;</div><div> }</div><div> </div></div><div>Thanks for maintaining the current interface. Now, I think we should change that interface :)</div><div><br></div>
<div>
The "only fix the main file" approach was me being paranoid when fix-its first went it. I think the right answer now is for -fixit to always apply changes to all of the files (in place), and to allow a file name suffix, e.g.,</div>
<div><br></div><div><span style="white-space: pre;"> </span>1) -fixit changes files in-place</div><div><span style="white-space: pre;"> </span>2) -fixit=suffix puts the changes for any file "foo.c" into "foosuffix.c"; we'll mainly use this for testing, I think.</div>
<div><br></div><div>At this point, I trust fix-its enough that I'd like to add -fixit to my normal Clang build. I'm not worried about it because we should only write out the changed files if *all* errors have fix-its that were properly applied, so we only end up changing the source code when our changes produced a program that actually compiles.</div>
</div></div></div></blockquote><div><br>Yes, this sounds like the right way to go forward. I'll be happy to rip out the -fixit-at logic!<br><br>Nick<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div style="word-wrap: break-word;"><div><div><div><br></div><div>Anyway, thank you for working on this! Please feel free to go ahead and commit with the few changes requested above; we can tackle the interface change separately.</div>
<div><br></div><div><span style="white-space: pre;"> </span>- Doug</div></div></div></div></blockquote></div><br>