NAKAMURA Takumi
2012-May-29 23:12 UTC
[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
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 >> >
Manuel Klimek
2012-May-30 07:38 UTC
[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
Michael pointed out in IRC that we have unique_file in v2 which should replace this. I'll change the code. On Wed, May 30, 2012 at 1:12 AM, NAKAMURA Takumi <geek4civic at gmail.com>wrote:> 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 > >> > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120530/a3a636c1/attachment.html>