Remove gmx_unique_ptr from options public headers

There was a single factory function in the generic option interface that
created objects and passed ownership to the caller.  The implementation
for this method always follows the same pattern, and there only a single
caller, so it is easy to make it exception-safe while returning a plain
pointer.  As a side effect, it removes some boilerplate code from all
the implementations for this method.

There are more complicated alternatives that avoid the plain pointer,
based on creating a helper class that can only be forward declared in
the installed headers, but I don't think those are worth the extra code.

This removes the last use of gmx_unique_ptr in an installed header.

Part of #1454

Change-Id: I948973d32042d388341265d5db6efbbcf0f7db8f

diff --git a/src/gromacs/options/abstractoption.h b/src/gromacs/options/abstractoption.h
index 4c0153b834b65a72d891c1db069715bc44883ba1..1152081eca90101c5364fd9c271bc40f9bf3cdb2 100644 (file)
--- a/src/gromacs/options/abstractoption.h
+++ b/src/gromacs/options/abstractoption.h
@@ -58,7 +58,6 @@
 #include <vector>
 
 #include "../utility/common.h"
-#include "../utility/uniqueptr.h"
 
 #include "optionflags.h"
 
@@ -70,10 +69,6 @@ template <typename T> class OptionStorageTemplate;
 class OptionManagerContainer;
 class Options;
 
-//! Smart pointer for managing an AbstractOptionStorage object.
-typedef gmx_unique_ptr<AbstractOptionStorage>::type
-    AbstractOptionStoragePointer;
-
 /*! \brief
  * Abstract base class for specifying option properties.
  *
@@ -122,8 +117,15 @@ class AbstractOption
          * They should also throw APIError if they detect problems.
          *
          * Should only be called by Options::addOption().
+         *
+         * The ownership of the return value is passed, but is not using a
+         * smart pointer to avoid introducing such a dependency in an installed
+         * header.  The implementation will always consist of a single `new`
+         * call and returning that value, and the caller always immediately
+         * wraps the pointer in a smart pointer, so there is not exception
+         * safety issue.
          */
-        virtual AbstractOptionStoragePointer createStorage(
+        virtual AbstractOptionStorage *createStorage(
             const OptionManagerContainer &managers) const = 0;
 
         //! Sets the description for the option.

diff --git a/src/gromacs/options/basicoptions.cpp b/src/gromacs/options/basicoptions.cpp
index 205a3c102334b72dd5ca6e5dc83fce4a69df175b..30eb023026e996d9891984cc7359118fb107be1a 100644 (file)
--- a/src/gromacs/options/basicoptions.cpp
+++ b/src/gromacs/options/basicoptions.cpp
@@ -138,10 +138,10 @@ bool BooleanOptionInfo::defaultValue() const
  * BooleanOption
  */
 
-AbstractOptionStoragePointer
+AbstractOptionStorage *
 BooleanOption::createStorage(const OptionManagerContainer & /*managers*/) const
 {
-    return AbstractOptionStoragePointer(new BooleanOptionStorage(*this));
+    return new BooleanOptionStorage(*this);
 }
 
 
@@ -196,10 +196,10 @@ IntegerOptionInfo::IntegerOptionInfo(IntegerOptionStorage *option)
  * IntegerOption
  */
 
-AbstractOptionStoragePointer
+AbstractOptionStorage *
 IntegerOption::createStorage(const OptionManagerContainer & /*managers*/) const
 {
-    return AbstractOptionStoragePointer(new IntegerOptionStorage(*this));
+    return new IntegerOptionStorage(*this);
 }
 
 
@@ -244,10 +244,10 @@ Int64OptionInfo::Int64OptionInfo(Int64OptionStorage *option)
  * Int64Option
  */
 
-AbstractOptionStoragePointer
+AbstractOptionStorage *
 Int64Option::createStorage(const OptionManagerContainer & /*managers*/) const
 {
-    return AbstractOptionStoragePointer(new Int64OptionStorage(*this));
+    return new Int64OptionStorage(*this);
 }
 
 
@@ -346,10 +346,10 @@ void DoubleOptionInfo::setScaleFactor(double factor)
  * DoubleOption
  */
 
-AbstractOptionStoragePointer
+AbstractOptionStorage *
 DoubleOption::createStorage(const OptionManagerContainer & /*managers*/) const
 {
-    return AbstractOptionStoragePointer(new DoubleOptionStorage(*this));
+    return new DoubleOptionStorage(*this);
 }
 
 
@@ -450,10 +450,10 @@ void FloatOptionInfo::setScaleFactor(double factor)
  * FloatOption
  */
 
-AbstractOptionStoragePointer
+AbstractOptionStorage *
 FloatOption::createStorage(const OptionManagerContainer & /*managers*/) const
 {
-    return AbstractOptionStoragePointer(new FloatOptionStorage(*this));
+    return new FloatOptionStorage(*this);
 }
 
 
@@ -636,10 +636,10 @@ const std::vector<std::string> &StringOptionInfo::allowedValues() const
  * StringOption
  */
 
-AbstractOptionStoragePointer
+AbstractOptionStorage *
 StringOption::createStorage(const OptionManagerContainer & /*managers*/) const
 {
-    return AbstractOptionStoragePointer(new StringOptionStorage(*this));
+    return new StringOptionStorage(*this);
 }
 
 } // namespace gmx

diff --git a/src/gromacs/options/basicoptions.h b/src/gromacs/options/basicoptions.h
index 992d19ed009db906e9fb5f092302b4a3cd4b655e..c35ee9228bef1768e6a2a8b4b314d8905f0d3276 100644 (file)
--- a/src/gromacs/options/basicoptions.h
+++ b/src/gromacs/options/basicoptions.h
@@ -98,7 +98,7 @@ class BooleanOption : public OptionTemplate<bool, BooleanOption>
 
     private:
         //! Creates a BooleanOptionStorage object.
-        virtual AbstractOptionStoragePointer createStorage(
+        virtual AbstractOptionStorage *createStorage(
             const OptionManagerContainer &managers) const;
 };
 
@@ -141,7 +141,7 @@ class IntegerOption : public OptionTemplate<int, IntegerOption>
 
     private:
         //! Creates an IntegerOptionStorage object.
-        virtual AbstractOptionStoragePointer createStorage(
+        virtual AbstractOptionStorage *createStorage(
             const OptionManagerContainer &managers) const;
 
         /*! \brief
@@ -171,7 +171,7 @@ class Int64Option : public OptionTemplate<gmx_int64_t, Int64Option>
 
     private:
         //! Creates an Int64OptionStorage object.
-        virtual AbstractOptionStoragePointer createStorage(
+        virtual AbstractOptionStorage *createStorage(
             const OptionManagerContainer &managers) const;
 
         /*! \brief
@@ -217,7 +217,7 @@ class DoubleOption : public OptionTemplate<double, DoubleOption>
 
     private:
         //! Creates a DoubleOptionStorage object.
-        virtual AbstractOptionStoragePointer createStorage(
+        virtual AbstractOptionStorage *createStorage(
             const OptionManagerContainer &managers) const;
 
         bool bTime_;
@@ -256,7 +256,7 @@ class FloatOption : public OptionTemplate<float, FloatOption>
 
     private:
         //! Creates a FloatOptionStorage object.
-        virtual AbstractOptionStoragePointer createStorage(
+        virtual AbstractOptionStorage *createStorage(
             const OptionManagerContainer &managers) const;
 
         bool bTime_;
@@ -377,7 +377,7 @@ class StringOption : public OptionTemplate<std::string, StringOption>
 
     private:
         //! Creates a StringOptionStorage object.
-        virtual AbstractOptionStoragePointer createStorage(
+        virtual AbstractOptionStorage *createStorage(
             const OptionManagerContainer &managers) const;
 
         const char *const      *enumValues_;

diff --git a/src/gromacs/options/filenameoption.cpp b/src/gromacs/options/filenameoption.cpp
index e821eed476488882f5aada88c03777fc4d5c02cd..5bc9da6cc6a5beb034b79f0bea4ab38ff22a55ee 100644 (file)
--- a/src/gromacs/options/filenameoption.cpp
+++ b/src/gromacs/options/filenameoption.cpp
@@ -481,11 +481,10 @@ FileNameOptionInfo::ExtensionList FileNameOptionInfo::extensions() const
  * FileNameOption
  */
 
-AbstractOptionStoragePointer
+AbstractOptionStorage *
 FileNameOption::createStorage(const OptionManagerContainer &managers) const
 {
-    return AbstractOptionStoragePointer(
-            new FileNameOptionStorage(*this, managers.get<FileNameOptionManager>()));
+    return new FileNameOptionStorage(*this, managers.get<FileNameOptionManager>());
 }
 
 } // namespace gmx

diff --git a/src/gromacs/options/filenameoption.h b/src/gromacs/options/filenameoption.h
index d8843cb38c3154867581ad414899fcb5a9b20259..e3ab252f7a615465cd97bbf6fdf37a3ffaa87e4a 100644 (file)
--- a/src/gromacs/options/filenameoption.h
+++ b/src/gromacs/options/filenameoption.h
@@ -166,7 +166,7 @@ class FileNameOption : public OptionTemplate<std::string, FileNameOption>
         using MyBase::defaultValueIfSet;
 
         //! Creates a FileNameOptionStorage object.
-        virtual AbstractOptionStoragePointer createStorage(
+        virtual AbstractOptionStorage *createStorage(
             const OptionManagerContainer &managers) const;
 
         OptionFileType          optionType_;

diff --git a/src/gromacs/options/options-impl.h b/src/gromacs/options/options-impl.h
index c126f07e8579236a660aa2c9ca46a3d6daee7a71..ce23aad95212ad4e71fe5a038940c4162305a041 100644 (file)
--- a/src/gromacs/options/options-impl.h
+++ b/src/gromacs/options/options-impl.h
@@ -48,6 +48,7 @@
 #include "abstractoption.h"
 #include "optionmanagercontainer.h"
 #include "options.h"
+#include "gromacs/utility/uniqueptr.h"
 
 namespace gmx
 {
@@ -66,6 +67,9 @@ class AbstractOptionStorage;
 class Options::Impl
 {
     public:
+        //! Smart pointer for managing an AbstractOptionStorage object.
+        typedef gmx_unique_ptr<AbstractOptionStorage>::type
+            AbstractOptionStoragePointer;
         //! Convenience type for list of sections.
         typedef std::vector<Options *> SubSectionList;
         //! Convenience type for list of options.

diff --git a/src/gromacs/options/options.cpp b/src/gromacs/options/options.cpp
index b64f285268d408a51d31db01e27301eeb59390c6..b208ecb7442211ed124b5873a378f0027935ef7c 100644 (file)
--- a/src/gromacs/options/options.cpp
+++ b/src/gromacs/options/options.cpp
@@ -195,7 +195,7 @@ OptionInfo *Options::addOption(const AbstractOption &settings)
     {
         root = root->parent_->impl_.get();
     }
-    AbstractOptionStoragePointer option(settings.createStorage(root->managers_));
+    Impl::AbstractOptionStoragePointer option(settings.createStorage(root->managers_));
     if (impl_->findOption(option->name().c_str()) != NULL)
     {
         GMX_THROW(APIError("Duplicate option: " + option->name()));

diff --git a/src/gromacs/options/tests/abstractoptionstorage.cpp b/src/gromacs/options/tests/abstractoptionstorage.cpp
index 6df36ee6eb63c1f1654232ca282de8a37bf1ecca..ed1b4c6c2116ed1fea287520ec1a338302a03620 100644 (file)
--- a/src/gromacs/options/tests/abstractoptionstorage.cpp
+++ b/src/gromacs/options/tests/abstractoptionstorage.cpp
@@ -151,10 +151,10 @@ class MockOption : public gmx::OptionTemplate<std::string, MockOption>
         }
 
     private:
-        virtual gmx::AbstractOptionStoragePointer createStorage(
+        virtual gmx::AbstractOptionStorage *createStorage(
             const gmx::OptionManagerContainer & /*managers*/) const
         {
-            return gmx::AbstractOptionStoragePointer(new MockOptionStorage(*this));
+            return new MockOptionStorage(*this);
         }
 };
 

diff --git a/src/gromacs/selection/selectionfileoption.h b/src/gromacs/selection/selectionfileoption.h
index 773bff96b3821a77a663aa5946537dc0d888c24f..324d33fdc7f84ffefc4be373c6ecab1a3b021e2d 100644 (file)
--- a/src/gromacs/selection/selectionfileoption.h
+++ b/src/gromacs/selection/selectionfileoption.h
@@ -76,7 +76,7 @@ class SelectionFileOption : public AbstractOption
         explicit SelectionFileOption(const char *name);
 
     private:
-        virtual AbstractOptionStoragePointer createStorage(
+        virtual AbstractOptionStorage *createStorage(
             const OptionManagerContainer &managers) const;
 };
 

diff --git a/src/gromacs/selection/selectionoption.cpp b/src/gromacs/selection/selectionoption.cpp
index 62fbc9cf50d32933cf5ebb61dfb30493f34ba0ba..4063cc85a3cbfb48754e8dc50d2cce910ced7031 100644 (file)
--- a/src/gromacs/selection/selectionoption.cpp
+++ b/src/gromacs/selection/selectionoption.cpp
@@ -250,12 +250,11 @@ void SelectionOptionInfo::setDynamicMask(bool bEnabled)
  * SelectionOption
  */
 
-AbstractOptionStoragePointer
+AbstractOptionStorage *
 SelectionOption::createStorage(const OptionManagerContainer &managers) const
 {
-    return AbstractOptionStoragePointer(
-            new SelectionOptionStorage(
-                    *this, managers.get<SelectionOptionManager>()));
+    return new SelectionOptionStorage(
+            *this, managers.get<SelectionOptionManager>());
 }
 
 
@@ -318,12 +317,11 @@ SelectionFileOption::SelectionFileOption(const char *name)
     setDescription("Provide selections from files");
 }
 
-AbstractOptionStoragePointer
+AbstractOptionStorage *
 SelectionFileOption::createStorage(const OptionManagerContainer &managers) const
 {
-    return AbstractOptionStoragePointer(
-            new SelectionFileOptionStorage(
-                    *this, managers.get<SelectionOptionManager>()));
+    return new SelectionFileOptionStorage(
+            *this, managers.get<SelectionOptionManager>());
 }
 
 } // namespace gmx

diff --git a/src/gromacs/selection/selectionoption.h b/src/gromacs/selection/selectionoption.h
index a67e4e337ba1d975942d26120b81d32432e91318..4655585f37ce9b180e3b7a4807c6b1bedb90734a 100644 (file)
--- a/src/gromacs/selection/selectionoption.h
+++ b/src/gromacs/selection/selectionoption.h
@@ -149,7 +149,7 @@ class SelectionOption : public OptionTemplate<Selection, SelectionOption>
         using MyBase::defaultValue;
         using MyBase::defaultValueIfSet;
 
-        virtual AbstractOptionStoragePointer createStorage(
+        virtual AbstractOptionStorage *createStorage(
             const OptionManagerContainer &managers) const;
 
         const char             *defaultText_;

diff --git a/src/testutils/integrationtests.cpp b/src/testutils/integrationtests.cpp
index 89845be1e5086a1f71c9446b17004e312fb14798..437dd1688578b51145d407d5d1edc3d0c0756fa8 100644 (file)
--- a/src/testutils/integrationtests.cpp
+++ b/src/testutils/integrationtests.cpp
@@ -50,6 +50,8 @@
 #include <stdlib.h>
 #include <stdio.h>
 
+#include "config.h"
+
 namespace gmx
 {
 namespace test