<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. Also, shouldn't the raw_ostream be a reference parameter rather than a pointer parameter, since it can't be NULL anyway?</div>
<div><br></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><br></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></div></div></blockquote><div><br>How does the testsuite work with this system?<br><br>The problem I'm having is that we can no longer run "clang -cc1 -fixit -o - | clang -verify". So we either run '-fixit' which modifies the test file in-place (no good), or we run '-fixit=fixed' which creates (for example) fixit-cxx0x.fixed.cpp. So far so good...<br>
<br>But we then want to check its contents somehow. To do that, I hard-code the full name of the test hard-coded (fixit-cxx0x.fixed.cpp) when what we really want is a %t style temporary file. We also need to 'RUN: rm' it afterwards, but if the test fails then we stop executing run lines and the file will be left behind.<br>
<br>Please let me know if there's something I'm missing. The patch which implements this (sans testsuite changes) is attached.<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></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><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>