r207396 - Follow-up to r207071: Let newFrontendActionFactory() return a unique_ptr.

[David Blaikie]

On Mon, Apr 28, 2014 at 11:04 AM, Nico Weber <thakis at chromium.org> wrote:
> On Mon, Apr 28, 2014 at 10:28 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Apr 27, 2014 10:06 PM, "Nico Weber" <nicolasweber at 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 at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>