[LLVMdev] [cfe-commits] r157260 - in /cfe/trunk: include/clang/Rewrite/Rewriter.h lib/Rewrite/Rewriter.cpp unittests/CMakeLists.txt unittests/Tooling/RewriterTest.cpp unittests/Tooling/RewriterTestContext.h

[NAKAMURA Takumi]

Manuel,

After the discussion at last night, I have agreed that
GetTemporaryDirectory() on Win32 would do bad thing, thank you.

dir = GetTemporaryDirectory();
dir.eraseFromDisk(erase_contents = true);
dir = GetTemporaryDirectory(); /* It doesn't create anything on Win32
due to caching */

I suppose Manuel wants GetTemporaryDirectory() to keep semantics
similar mkdtemp(3).

Though it is in PathV1, could we fix?

...Takumi

2012/5/29 Manuel Klimek <klimek at google.com>:
> Additionally, from looking at the PathV1.h comments, it looks like the
> Windows version actually violates the contract "@brief Construct a path to
> an new, unique, existing temporary directory."
>
> Cheers,
> /Manuel
>
>
> On Tue, May 29, 2012 at 8:50 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>> On Sun, May 27, 2012 at 4:01 PM, NAKAMURA Takumi <geek4civic at gmail.com>
>> wrote:
>>>
>>> 2012/5/23 Manuel Klimek <klimek at google.com>:
>>> > Author: klimek
>>> > Date: Tue May 22 12:01:35 2012
>>> > New Revision: 157260
>>> >
>>> > URL: http://llvm.org/viewvc/llvm-project?rev=157260&view=rev
>>> > Log:
>>> > Adds a method overwriteChangedFiles to the Rewriter. This is
>>> > implemented by
>>> > first writing the changed files to a temporary location and then
>>> > overwriting
>>> > the original files atomically.
>>> >
>>> > Also adds a RewriterTestContext to aid unit testing rewrting logic in
>>> > general.
>>> >
>>> >
>>> > Added:
>>> >    cfe/trunk/unittests/Tooling/RewriterTest.cpp   (with props)
>>> >    cfe/trunk/unittests/Tooling/RewriterTestContext.h   (with props)
>>> > Modified:
>>> >    cfe/trunk/include/clang/Rewrite/Rewriter.h
>>> >    cfe/trunk/lib/Rewrite/Rewriter.cpp
>>> >    cfe/trunk/unittests/CMakeLists.txt
>>>
>>>
>>> > Added: cfe/trunk/unittests/Tooling/RewriterTestContext.h
>>> > URL:
>>> > http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/RewriterTestContext.h?rev=157260&view=auto
>>> >
>>> > ==============================================================================
>>> > --- cfe/trunk/unittests/Tooling/RewriterTestContext.h (added)
>>> > +++ cfe/trunk/unittests/Tooling/RewriterTestContext.h Tue May 22
>>> > 12:01:35 2012
>>> > @@ -0,0 +1,120 @@
>>> > +//===--- RewriterTestContext.h ----------------------------------*-
>>> > C++ -*-===//
>>> > +//
>>> > +//                     The LLVM Compiler Infrastructure
>>> > +//
>>> > +// This file is distributed under the University of Illinois Open
>>> > Source
>>> > +// License. See LICENSE.TXT for details.
>>> > +//
>>> >
>>> > +//===----------------------------------------------------------------------===//
>>> > +//
>>> > +//  This file defines a utility class for Rewriter related tests.
>>> > +//
>>> >
>>> > +//===----------------------------------------------------------------------===//
>>> > +
>>> > +#ifndef LLVM_CLANG_REWRITER_TEST_CONTEXT_H
>>> > +#define LLVM_CLANG_REWRITER_TEST_CONTEXT_H
>>> > +
>>> > +#include "clang/Basic/Diagnostic.h"
>>> > +#include "clang/Basic/FileManager.h"
>>> > +#include "clang/Basic/LangOptions.h"
>>> > +#include "clang/Basic/SourceManager.h"
>>> > +#include "clang/Frontend/DiagnosticOptions.h"
>>> > +#include "clang/Frontend/TextDiagnosticPrinter.h"
>>> > +#include "clang/Rewrite/Rewriter.h"
>>> > +#include "llvm/Support/Path.h"
>>> > +#include "llvm/Support/raw_ostream.h"
>>> > +
>>> > +namespace clang {
>>> > +
>>> > +/// \brief A class that sets up a ready to use Rewriter.
>>> > +///
>>> > +/// Useful in unit tests that need a Rewriter. Creates all
>>> > dependencies
>>> > +/// of a Rewriter with default values for testing and provides
>>> > convenience
>>> > +/// methods, which help with writing tests that change files.
>>> > +class RewriterTestContext {
>>> > + public:
>>> > +  RewriterTestContext()
>>> > +      : Diagnostics(llvm::IntrusiveRefCntPtr<DiagnosticIDs>()),
>>> > +        DiagnosticPrinter(llvm::outs(), DiagnosticOptions()),
>>> > +        Files((FileSystemOptions())),
>>> > +        Sources(Diagnostics, Files),
>>> > +        Rewrite(Sources, Options) {
>>> > +    Diagnostics.setClient(&DiagnosticPrinter, false);
>>> > +  }
>>> > +
>>> > +  ~RewriterTestContext() {
>>> > +    if (TemporaryDirectory.isValid()) {
>>> > +      std::string ErrorInfo;
>>> > +      TemporaryDirectory.eraseFromDisk(true, &ErrorInfo);
>>> > +      assert(ErrorInfo.empty());
>>> > +    }
>>> > +  }
>>>
>>> Don't try to remove the TemporaryDirectory given by PathV1. Fixed in
>>> r157530.
>>> See also r157529.
>>
>>
>> The Windows and Unix implementation of GetTemporaryDirectory seem to
>> disagree on the contract here.
>> From what I can see the Unix implementation neither reuses the path, nor
>> deletes it on exit (both need to be done).
>>
>> Why does the Windows implementation re-use the temporary directory?
>>
>> Cheers,
>> /Manuel
>>
>