[cfe-commits] multifile fixits
Chandler Carruth
chandlerc at google.com
Fri Apr 23 01:23:54 PDT 2010
On Fri, Apr 23, 2010 at 12:47 AM, Nick Lewycky <nlewycky at google.com> wrote:
> 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.
>
Maybe I'm just too unix-y, but I wonder if it would help to extract the
logic of re-writing the file from Clang proper. If we had -fixit-diff output
a unified diff the tests can use FileCheck to ensure the diff contains
appropriate edits, the users can verify changes or prepare patches for code
review trivially, and updating the file becomes 'clang -fixit-diff | patch
-p0'. The logic in patch should even correctly handle the most obvious cases
of two patches being applied simultaneously.
Thoughts? The biggest concern I would have is portability, and I'm not
opposed to providing built-in logic to apply the patch directly within Clang
if needed based on the same structure that we use to build the diff.
::shrug:: I've definitely heard requests for a unified diff of the changes
fixit is about to make when telling folks about it here.
>
> 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
>>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100423/7c1facde/attachment.html>
More information about the cfe-commits
mailing list