[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

[Douglas Gregor]

On Apr 1, 2009, at 9:58 PM, Chris Lattner wrote:

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

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

Ok.

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

OutFile might be &llvm::outs(), in which case it's a good idea to flush.

>> +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 :)

Alright, which joker put Fix-It on the critical path?

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

Yay!

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

Yes, it should.

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

> This is cool stuff Doug, very nice!

Thanks for the review!

	- Doug