On Mon, Apr 28, 2014 at 11:04 AM, Nico Weber <tha...@chromium.org> wrote: > On Mon, Apr 28, 2014 at 10:28 AM, David Blaikie <dblai...@gmail.com> wrote: >> >> On Apr 27, 2014 10:06 PM, "Nico Weber" <nicolaswe...@gmx.de> wrote: >>> >>> Author: nico >>> Date: Sun Apr 27 23:57:14 2014 >>> New Revision: 207396 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=207396&view=rev >>> Log: >>> Follow-up to r207071: Let newFrontendActionFactory() return a unique_ptr. >>> >>> This exposed a leak, fix that. >>> >>> Modified: >>> cfe/trunk/include/clang/Tooling/Tooling.h >>> cfe/trunk/tools/clang-check/ClangCheck.cpp >>> >>> Modified: cfe/trunk/include/clang/Tooling/Tooling.h >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Tooling.h?rev=207396&r1=207395&r2=207396&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/include/clang/Tooling/Tooling.h (original) >>> +++ cfe/trunk/include/clang/Tooling/Tooling.h Sun Apr 27 23:57:14 2014 >>> @@ -97,7 +97,7 @@ public: >>> /// FrontendActionFactory *Factory = >>> /// newFrontendActionFactory<clang::SyntaxOnlyAction>(); >>> template <typename T> >>> -FrontendActionFactory *newFrontendActionFactory(); >>> +std::unique_ptr<FrontendActionFactory> newFrontendActionFactory(); >>> >>> /// \brief Callbacks called before and after each source file processed >>> by a >>> /// FrontendAction created by the FrontedActionFactory returned by \c >>> @@ -126,10 +126,10 @@ public: >>> /// struct ProvidesASTConsumers { >>> /// clang::ASTConsumer *newASTConsumer(); >>> /// } Factory; >>> -/// FrontendActionFactory *FactoryAdapter = >>> -/// newFrontendActionFactory(&Factory); >>> +/// std::unique_ptr<FrontendActionFactory> FactoryAdapter( >>> +/// newFrontendActionFactory(&Factory)); >>> template <typename FactoryT> >>> -inline FrontendActionFactory *newFrontendActionFactory( >>> +inline std::unique_ptr<FrontendActionFactory> newFrontendActionFactory( >>> FactoryT *ConsumerFactory, SourceFileCallbacks *Callbacks = NULL); >>> >>> /// \brief Runs (and deletes) the tool on 'Code' with the -fsyntax-only >>> flag. >>> @@ -305,17 +305,18 @@ class ClangTool { >>> }; >>> >>> template <typename T> >>> -FrontendActionFactory *newFrontendActionFactory() { >>> +std::unique_ptr<FrontendActionFactory> newFrontendActionFactory() { >>> class SimpleFrontendActionFactory : public FrontendActionFactory { >>> public: >>> clang::FrontendAction *create() override { return new T; } >>> }; >>> >>> - return new SimpleFrontendActionFactory; >>> + return std::unique_ptr<FrontendActionFactory>( >>> + new SimpleFrontendActionFactory); >>> } >>> >>> template <typename FactoryT> >>> -inline FrontendActionFactory *newFrontendActionFactory( >>> +inline std::unique_ptr<FrontendActionFactory> newFrontendActionFactory( >>> FactoryT *ConsumerFactory, SourceFileCallbacks *Callbacks) { >>> class FrontendActionFactoryAdapter : public FrontendActionFactory { >>> public: >>> @@ -362,7 +363,8 @@ inline FrontendActionFactory *newFronten >>> SourceFileCallbacks *Callbacks; >>> }; >>> >>> - return new FrontendActionFactoryAdapter(ConsumerFactory, Callbacks); >>> + return std::unique_ptr<FrontendActionFactory>( >>> + new FrontendActionFactoryAdapter(ConsumerFactory, Callbacks)); >> >> Could you use make_unique here? (one reason it's not possible to use that is >> when the ctor is private/friended, etc) > > It's possible; I just didn't know we have make_unique in llvm :-) > (There are no callers of it in clang at the moment.) > > However, it doesn't seem much shorter in this case ( > http://codepad.org/ucgK1YdG ) as it requires an additional include > line, and for functions that return Superclass in the interface but > Subclass in the return statement (like here) there's an additional > temporary object, so I'm not sure if it's better in this case?
Personally, I'd strongly prefer using make_unique wherever possible. while "x.reset(new T())" isn't too bad - the ability to treat any raw "new" with nearly as much suspicion as raw "delete" is a rather nice property to have. > >> >>> } >>> >>> /// \brief Returns the absolute path of \c File, by prepending it with >>> >>> Modified: cfe/trunk/tools/clang-check/ClangCheck.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/clang-check/ClangCheck.cpp?rev=207396&r1=207395&r2=207396&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/tools/clang-check/ClangCheck.cpp (original) >>> +++ cfe/trunk/tools/clang-check/ClangCheck.cpp Sun Apr 27 23:57:14 2014 >>> @@ -215,7 +215,7 @@ int main(int argc, const char **argv) { >>> Analyze ? "--analyze" : "-fsyntax-only", InsertAdjuster::BEGIN)); >>> >>> clang_check::ClangCheckActionFactory CheckFactory; >>> - FrontendActionFactory *FrontendFactory; >>> + std::unique_ptr<FrontendActionFactory> FrontendFactory; >>> >>> // Choose the correct factory based on the selected mode. >>> if (Analyze) >>> @@ -225,5 +225,5 @@ int main(int argc, const char **argv) { >>> else >>> FrontendFactory = newFrontendActionFactory(&CheckFactory); >>> >>> - return Tool.run(FrontendFactory); >>> + return Tool.run(FrontendFactory.get()); >>> } >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> cfe-commits@cs.uiuc.edu >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
