[cfe-commits] multifile fixits

[Nick Lewycky]

On 14 April 2010 07:44, Douglas Gregor <dgregor at apple.com> wrote:

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

How does the testsuite work with this system?

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

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.

Please let me know if there's something I'm missing. The patch which
implements this (sans testsuite changes) is attached.

Nick


> 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/20100423/db36f44f/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-new-fixit.patch
Type: text/x-diff
Size: 14536 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100423/db36f44f/attachment.patch>