[cfe-commits] r68268 - in /cfe/trunk: include/clang/Frontend/FixItRewriter.h lib/Frontend/FixItRewriter.cpp lib/Parse/ParseInit.cpp lib/Sema/SemaDeclCXX.cpp test/Sema/fixit-c90.c test/Sema/fixit-errors.c test/Sema/fixit.c test/SemaCXX/fixit.cpp tools/clang-cc/clang-cc.cpp

[Chris Lattner]

On Apr 1, 2009, at 6:08 PM, Douglas Gregor wrote:
> Author: dgregor
> Date: Wed Apr  1 20:08:08 2009
> New Revision: 68268
>
> URL: http://llvm.org/viewvc/llvm-project?rev=68268&view=rev
> Log:
> Introduce a "-fixit" mode to clang-cc that applies code-modification  
> hints.

I was just going to say "we really need some way to test these fixit  
hints".  This is a great answer to that predictable request :)

> +++ cfe/trunk/lib/Frontend/FixItRewriter.cpp Wed Apr  1 20:08:08 2009
> @@ -0,0 +1,140 @@
> +//===--- FixItRewriter.cpp - Fix-It Rewriter Diagnostic Client --*-  
> C++ -*-===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open  
> Source
> +// License. See LICENSE.TXT for details.
> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +//
> +// This is a diagnostic client adaptor that performs rewrites as
> +// suggested by code modification hints attached to diagnostics. It
> +// then forwards any diagnostics to the adapted diagnostic client.
> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +#include "clang/Basic/SourceManager.h"
> +#include "clang/Frontend/FixItRewriter.h"

Please include FixItRewriter.h first, since this is the .cpp file that  
corresponds to it.  Also, a space between the header comment and first  
#include would be nice :)

> +bool FixItRewriter::WriteFixedFile(const std::string &InFileName,
> +                                   const std::string &OutFileName) {
> +
> +  llvm::OwningPtr<llvm::raw_ostream> OwnedStream;
> +  llvm::raw_ostream *OutFile;
> +  if (OutFileName == "-") {
> +    OutFile = &llvm::outs();
> +  } else if (!OutFileName.empty()) {
> +    std::string Err;
> +    OutFile = new llvm::raw_fd_ostream(OutFileName.c_str(),
> +                                       // set binary mode (critical  
> for Windoze)
> +                                       true,
> +                                       Err);

This ctor for raw_fd_ostream directly supports "-", so you can drop  
the first if.

> +  FileID MainFileID = Rewrite->getSourceMgr().getMainFileID();
> +  if (const RewriteBuffer *RewriteBuf =
> +        Rewrite->getRewriteBufferFor(MainFileID)) {
> +    *OutFile << std::string(RewriteBuf->begin(), RewriteBuf->end());
> +  } else {
> +    std::fprintf(stderr, "Main file is unchanged\n");

If the main file is unchanged, shouldn't this be unreached?

>
> +  }
> +  OutFile->flush();

Should OutFile be flushed or deleted here?  If OwnedStream will  
destroy it, then it doesn't need to be flushed.

> +void FixItRewriter::HandleDiagnostic(Diagnostic::Level DiagLevel,
> +                                     const DiagnosticInfo &Info) {
> +  if (Client)
> +    Client->HandleDiagnostic(DiagLevel, Info);
> +
> +  // Make sure that we can perform all of the modifications we
> +  // in this diagnostic.
> +  bool CanRewrite = true;
> +  for (unsigned Idx = 0; Idx < Info.getNumCodeModificationHints(); + 
> +Idx) {

Please don't call Info.getNumCodeModificationHints() each time through  
the loop :)

> +    const CodeModificationHint &Hint =  
> Info.getCodeModificationHint(Idx);
> +    if (Hint.RemoveRange.isValid() &&
> +        (!Rewrite->isRewritable(Hint.RemoveRange.getBegin()) ||
> +         !Rewrite->isRewritable(Hint.RemoveRange.getEnd()) ||
> +         Rewrite->getRangeSize(Hint.RemoveRange) == -1)) {

getRangeSize returns -1 if either of the start/end are not rewritable,  
so you can drop those checks.
> +    if (Hint.InsertionLoc.isValid() &&
> +        !Rewrite->isRewritable(Hint.InsertionLoc)) {
> +      CanRewrite = false;
> +      break;
> +    }
> +  }

What if an error diagnostic has no rewrite info?  Shouldn't that cause  
the fixit rewriter to refuse to apply any rewrites?

> +
> +  if (!CanRewrite) // FIXME: warn the user that this rewrite  
> couldn't be done
> +    return;
> +
> +  bool Failed = false;
> +  for (unsigned Idx = 0; Idx < Info.getNumCodeModificationHints(); + 
> +Idx) {

No need to call Info.getNumCodeModificationHints() each time through  
the loop.

> +    const CodeModificationHint &Hint =  
> Info.getCodeModificationHint(Idx);
> +    if (Hint.RemoveRange.isValid()) {

Please handle the !valid case first, allowing you to turn the rest  
into elseif chains and reduce indentation.

This is cool stuff Doug, very nice!

-Chris