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

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.


> Also, shouldn't the raw_ostream be a reference parameter rather than a
> pointer parameter, since it can't be NULL anyway?
>

Done.


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

Done.


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

Yes, this sounds like the right way to go forward. I'll be happy to rip out
the -fixit-at logic!

Nick


>
> 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/e9d70897/attachment.html>