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

[Nico Weber]

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?

>
>>  }
>>
>>  /// \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
>