[cfe-commits] multifile fixits

[Douglas Gregor]

On Apr 9, 2010, at 7:24 PM, Nick Lewycky wrote:

> 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.
> 
> 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 foo.fixit.cc.


--- /tmp/g4-68652/cache/depot/google3/third_party/llvm/trunk/tools/clang/lib/Frontend/FixItRewriter.cpp#4	2010-04-08 23:10:18.000000000 -0700
+++ /home/nlewycky/llvm-ftw1/google3/third_party/llvm/trunk/tools/clang/lib/Frontend/FixItRewriter.cpp	2010-04-09 17:17:26.741262000 -0700
@@ -14,6 +14,8 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/Frontend/FixItRewriter.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "llvm/Support/raw_ostream.h"
@@ -34,47 +36,41 @@
   Diags.setClient(Client);
 }
 
-bool FixItRewriter::WriteFixedFile(const std::string &InFileName,
-                                   const std::string &OutFileName) {
+bool FixItRewriter::WriteFixedFile(FileID ID, llvm::raw_ostream *OS) {
+  const RewriteBuffer *RewriteBuf = Rewrite.getRewriteBufferFor(ID);
+  if (!RewriteBuf) return true;
+  *OS << std::string(RewriteBuf->begin(), RewriteBuf->end());
+  OS->flush();
+  return false;
+}

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?

+
+bool FixItRewriter::WriteFixedFiles() {
   if (NumFailures > 0) {
     Diag(FullSourceLoc(), diag::warn_fixit_no_changes);
     return true;
   }
 
-  llvm::OwningPtr<llvm::raw_ostream> OwnedStream;
-  llvm::raw_ostream *OutFile;
-  if (!OutFileName.empty()) {
-    std::string Err;
-    OutFile = new llvm::raw_fd_ostream(OutFileName.c_str(), Err,
-                                       llvm::raw_fd_ostream::F_Binary);
-    OwnedStream.reset(OutFile);
-  } else if (InFileName == "-") {
-    OutFile = &llvm::outs();
-  } else {
-    llvm::sys::Path Path(InFileName);
+  for (iterator I = buffer_begin(), E = buffer_end(); I != E; ++I) {
+    const FileEntry *Entry = Rewrite.getSourceMgr().getFileEntryForID(I->first);
+    llvm::sys::Path Path(Entry->getName());
     std::string Suffix = Path.getSuffix();
     Path.eraseSuffix();
     Path.appendSuffix("fixit." + Suffix);
     std::string Err;
-    OutFile = new llvm::raw_fd_ostream(Path.c_str(), Err,
-                                       llvm::raw_fd_ostream::F_Binary);
-    OwnedStream.reset(OutFile);
-  }
+    llvm::raw_fd_ostream *OS = new llvm::raw_fd_ostream(Path.c_str(), Err,
+                                                llvm::raw_fd_ostream::F_Binary);

The raw_fd_ostream could/should be stack-allocated.

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).

-  FileID MainFileID = Rewrite.getSourceMgr().getMainFileID();
-  if (const RewriteBuffer *RewriteBuf =
-        Rewrite.getRewriteBufferFor(MainFileID)) {
-    *OutFile << std::string(RewriteBuf->begin(), RewriteBuf->end());
-  } else {
-    Diag(FullSourceLoc(), diag::note_fixit_main_file_unchanged);
+    RewriteBuffer &RewriteBuf = I->second;
+    *OS << std::string(RewriteBuf.begin(), RewriteBuf.end());
+    OS->flush();
+    delete OS;

Same OS.write() comment as above.

==== //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 ====
--- /tmp/g4-68652/cache/depot/google3/third_party/llvm/trunk/tools/clang/lib/Frontend/FrontendActions.cpp#7	2010-04-08 21:49:14.000000000 -0700
+++ /home/nlewycky/llvm-ftw1/google3/third_party/llvm/trunk/tools/clang/lib/Frontend/FrontendActions.cpp	2010-04-09 15:59:12.323217000 -0700
@@ -139,6 +139,23 @@
     FixItRewrite.addFixItLocation(Requested);
   }
 
+  const std::string &OutputFile = CI.getFrontendOpts().OutputFile;
+  if (Locs.empty() && !OutputFile.empty()) {
+    // FIXME: we will issue "FIX-IT applied suggested code changes" for every
+    // input, but only the main file will actually be rewritten.
+    const std::vector<std::pair<FrontendOptions::InputKind, std::string> > &Inputs =
+      CI.getFrontendOpts().Inputs;
+    for (unsigned i = 0, e = Inputs.size(); i != e; ++i) {
+      const FileEntry *File = CI.getFileManager().getFile(Inputs[i].second);
+      assert(File && "Input file not found in FileManager");
+      RequestedSourceLocation Requested;
+      Requested.File = File;
+      Requested.Line = 0;
+      Requested.Column = 0;
+      FixItRewrite.addFixItLocation(Requested);
+    }
+  }
+
   return true;
 }
 
Thanks for maintaining the current interface. Now, I think we should change that interface :)

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.,

	1) -fixit changes files in-place
	2) -fixit=suffix puts the changes for any file "foo.c" into "foosuffix.c"; we'll mainly use this for testing, I think.

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.

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.

	- Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100414/77f2440e/attachment.html>