From 59f37924f3fe61eba5ffaabb6d1331b2567f3ee6 Mon Sep 17 00:00:00 2001 From: Teemu Murtola Date: Sun, 1 Nov 2015 08:46:41 +0200 Subject: [PATCH] Remove more scoped_ptr uses Either replace with move-constructable/-assignable types, or clarify the code to only initialize the pointers once. Make PrivateImplPointer easier to use for cases where it might be null. Change-Id: I95b5da73af69ece519ab694f9d100039ab512e68 --- src/gromacs/commandline/cmdlinehelpcontext.cpp | 12 ++++ src/gromacs/commandline/cmdlinehelpcontext.h | 4 ++ src/gromacs/commandline/cmdlinehelpmodule.cpp | 67 ++++++++++++++-------- .../commandline/tests/cmdlinemodulemanagertest.cpp | 31 +++++----- src/gromacs/selection/selectioncollection.cpp | 26 ++++++--- src/gromacs/utility/classhelpers.h | 15 ++++- src/gromacs/utility/datafilefinder.cpp | 12 ++-- src/gromacs/utility/directoryenumerator.cpp | 6 +- src/gromacs/utility/gmxregex.cpp | 4 +- 9 files changed, 118 insertions(+), 59 deletions(-) diff --git a/src/gromacs/commandline/cmdlinehelpcontext.cpp b/src/gromacs/commandline/cmdlinehelpcontext.cpp index d1af037ef5..a952c95945 100644 --- a/src/gromacs/commandline/cmdlinehelpcontext.cpp +++ b/src/gromacs/commandline/cmdlinehelpcontext.cpp @@ -124,6 +124,18 @@ CommandLineHelpContext::CommandLineHelpContext( { } +CommandLineHelpContext::CommandLineHelpContext(CommandLineHelpContext &&other) + : impl_(std::move(other.impl_)) +{ +} + +CommandLineHelpContext &CommandLineHelpContext::operator=( + CommandLineHelpContext &&other) +{ + impl_ = std::move(other.impl_); + return *this; +} + CommandLineHelpContext::~CommandLineHelpContext() { } diff --git a/src/gromacs/commandline/cmdlinehelpcontext.h b/src/gromacs/commandline/cmdlinehelpcontext.h index be59e70cbb..dbfd341bfa 100644 --- a/src/gromacs/commandline/cmdlinehelpcontext.h +++ b/src/gromacs/commandline/cmdlinehelpcontext.h @@ -87,6 +87,10 @@ class CommandLineHelpContext explicit CommandLineHelpContext(ShellCompletionWriter *writer); //! Creates a copy of the context. explicit CommandLineHelpContext(const CommandLineHelpContext &other); + //! Moves the context. + CommandLineHelpContext(CommandLineHelpContext &&other); + //! Move-assigns the context. + CommandLineHelpContext &operator=(CommandLineHelpContext &&other); ~CommandLineHelpContext(); /*! \brief diff --git a/src/gromacs/commandline/cmdlinehelpmodule.cpp b/src/gromacs/commandline/cmdlinehelpmodule.cpp index ed37e17d9e..5467375219 100644 --- a/src/gromacs/commandline/cmdlinehelpmodule.cpp +++ b/src/gromacs/commandline/cmdlinehelpmodule.cpp @@ -43,6 +43,7 @@ #include "cmdlinehelpmodule.h" +#include #include #include @@ -122,6 +123,8 @@ class RootHelpTopic : public AbstractCompositeHelpTopic // unused because of the writeHelp() override virtual std::string helpText() const { return ""; } + CommandLineHelpContext createContext(const HelpWriterContext &context) const; + const CommandLineHelpModuleImpl &helpModule_; std::string title_; std::vector exportedTopics_; @@ -143,6 +146,9 @@ class CommandLineHelpModuleImpl const CommandLineModuleMap &modules, const CommandLineModuleGroupList &groups); + std::unique_ptr createExporter( + const std::string &format, + IFileOutputRedirector *redirector); void exportHelp(IHelpExport *exporter); RootHelpTopic rootTopic_; @@ -286,17 +292,9 @@ void RootHelpTopic::exportHelp(IHelpExport *exporter) void RootHelpTopic::writeHelp(const HelpWriterContext &context) const { { - CommandLineCommonOptionsHolder optionsHolder; - boost::scoped_ptr cmdlineContext; - if (helpModule_.context_ != NULL) - { - cmdlineContext.reset(new CommandLineHelpContext(*helpModule_.context_)); - } - else - { - cmdlineContext.reset(new CommandLineHelpContext(context)); - } - cmdlineContext->setModuleDisplayName(helpModule_.binaryName_); + CommandLineCommonOptionsHolder optionsHolder; + CommandLineHelpContext cmdlineContext(createContext(context)); + cmdlineContext.setModuleDisplayName(helpModule_.binaryName_); optionsHolder.initOptions(); Options &options = *optionsHolder.options(); ConstArrayRef helpText; @@ -307,7 +305,7 @@ void RootHelpTopic::writeHelp(const HelpWriterContext &context) const // TODO: Add [] into the synopsis. CommandLineHelpWriter(options) .setHelpText(helpText) - .writeHelp(*cmdlineContext); + .writeHelp(cmdlineContext); } if (context.outputFormat() == eHelpOutputFormat_Console) { @@ -331,6 +329,19 @@ void RootHelpTopic::writeHelp(const HelpWriterContext &context) const } } +CommandLineHelpContext +RootHelpTopic::createContext(const HelpWriterContext &context) const +{ + if (helpModule_.context_ != NULL) + { + return CommandLineHelpContext(*helpModule_.context_); + } + else + { + return CommandLineHelpContext(context); + } +} + /******************************************************************** * CommandsHelpTopic */ @@ -768,6 +779,23 @@ CommandLineHelpModuleImpl::CommandLineHelpModuleImpl( { } +std::unique_ptr +CommandLineHelpModuleImpl::createExporter(const std::string &format, + IFileOutputRedirector *redirector) +{ + if (format == "rst") + { + return std::unique_ptr( + new HelpExportReStructuredText(*this, redirector)); + } + else if (format == "completion") + { + return std::unique_ptr( + new HelpExportCompletion(*this)); + } + GMX_THROW(NotImplementedError("This help format is not implemented")); +} + void CommandLineHelpModuleImpl::exportHelp(IHelpExport *exporter) { // TODO: Would be nicer to have the file names supplied by the build system @@ -928,19 +956,8 @@ int CommandLineHelpModule::run(int argc, char *argv[]) if (!exportFormat.empty()) { ModificationCheckingFileOutputRedirector redirector(impl_->outputRedirector_); - boost::scoped_ptr exporter; - if (exportFormat == "rst") - { - exporter.reset(new HelpExportReStructuredText(*impl_, &redirector)); - } - else if (exportFormat == "completion") - { - exporter.reset(new HelpExportCompletion(*impl_)); - } - else - { - GMX_THROW(NotImplementedError("This help format is not implemented")); - } + const std::unique_ptr exporter( + impl_->createExporter(exportFormat, &redirector)); impl_->exportHelp(exporter.get()); return 0; } diff --git a/src/gromacs/commandline/tests/cmdlinemodulemanagertest.cpp b/src/gromacs/commandline/tests/cmdlinemodulemanagertest.cpp index 4a785048bb..a868029462 100644 --- a/src/gromacs/commandline/tests/cmdlinemodulemanagertest.cpp +++ b/src/gromacs/commandline/tests/cmdlinemodulemanagertest.cpp @@ -46,13 +46,13 @@ #include #include -#include #include #include "gromacs/commandline/cmdlinehelpcontext.h" #include "gromacs/commandline/cmdlinemodule.h" #include "gromacs/commandline/cmdlinemodulemanager.h" #include "gromacs/commandline/cmdlineprogramcontext.h" +#include "gromacs/utility/gmxassert.h" #include "gromacs/utility/stringutil.h" #include "gromacs/onlinehelp/tests/mock_helptopic.h" @@ -132,13 +132,21 @@ MockOptionsModule::~MockOptionsModule() class CommandLineModuleManagerTestBase::Impl { public: - TestFileOutputRedirector redirector_; - boost::scoped_ptr programContext_; - boost::scoped_ptr manager_; + Impl(const CommandLine &args, const char *realBinaryName) + : programContext_(args.argc(), args.argv()), + manager_(realBinaryName, &programContext_) + { + manager_.setQuiet(true); + manager_.setOutputRedirector(&redirector_); + } + + TestFileOutputRedirector redirector_; + CommandLineProgramContext programContext_; + CommandLineModuleManager manager_; }; CommandLineModuleManagerTestBase::CommandLineModuleManagerTestBase() - : impl_(new Impl) + : impl_(nullptr) { } @@ -149,13 +157,8 @@ CommandLineModuleManagerTestBase::~CommandLineModuleManagerTestBase() void CommandLineModuleManagerTestBase::initManager( const CommandLine &args, const char *realBinaryName) { - impl_->manager_.reset(); - impl_->programContext_.reset( - new gmx::CommandLineProgramContext(args.argc(), args.argv())); - impl_->manager_.reset(new gmx::CommandLineModuleManager( - realBinaryName, impl_->programContext_.get())); - impl_->manager_->setQuiet(true); - impl_->manager_->setOutputRedirector(&impl_->redirector_); + GMX_RELEASE_ASSERT(impl_ == nullptr, "initManager() called more than once in test"); + impl_.reset(new Impl(args, realBinaryName)); } MockModule & @@ -186,11 +189,13 @@ CommandLineModuleManagerTestBase::addHelpTopic(const char *name, const char *tit CommandLineModuleManager &CommandLineModuleManagerTestBase::manager() { - return *impl_->manager_; + GMX_RELEASE_ASSERT(impl_ != nullptr, "initManager() not called"); + return impl_->manager_; } void CommandLineModuleManagerTestBase::checkRedirectedOutput() { + GMX_RELEASE_ASSERT(impl_ != nullptr, "initManager() not called"); impl_->redirector_.checkRedirectedFiles(&checker()); } diff --git a/src/gromacs/selection/selectioncollection.cpp b/src/gromacs/selection/selectioncollection.cpp index 90d5a89fa2..ffeb6d90f8 100644 --- a/src/gromacs/selection/selectioncollection.cpp +++ b/src/gromacs/selection/selectioncollection.cpp @@ -49,7 +49,6 @@ #include #include -#include #include #include "gromacs/fileio/trx.h" @@ -682,6 +681,23 @@ SelectionCollection::parseFromStdin(int count, bool bInteractive, context); } +namespace +{ + +//! Helper function to initialize status writer for interactive selection parsing. +std::unique_ptr initStatusWriter(TextOutputStream *statusStream) +{ + std::unique_ptr statusWriter; + if (statusStream != NULL) + { + statusWriter.reset(new TextWriter(statusStream)); + statusWriter->wrapperSettings().setLineLength(78); + } + return std::move(statusWriter); +} + +} // namespace + SelectionList SelectionCollection::parseInteractive(int count, TextInputStream *inputStream, @@ -690,12 +706,8 @@ SelectionCollection::parseInteractive(int count, { yyscan_t scanner; - boost::scoped_ptr statusWriter; - if (statusStream != NULL) - { - statusWriter.reset(new TextWriter(statusStream)); - statusWriter->wrapperSettings().setLineLength(78); - } + const std::unique_ptr statusWriter( + initStatusWriter(statusStream)); _gmx_sel_init_lexer(&scanner, &impl_->sc_, statusWriter.get(), count, impl_->bExternalGroupsSet_, impl_->grps_); return runParser(scanner, inputStream, true, count, context); diff --git a/src/gromacs/utility/classhelpers.h b/src/gromacs/utility/classhelpers.h index bead23491c..3ccfa4ebbe 100644 --- a/src/gromacs/utility/classhelpers.h +++ b/src/gromacs/utility/classhelpers.h @@ -132,6 +132,8 @@ template class PrivateImplPointer { public: + //! Allow implicit initialization from nullptr to support comparison. + PrivateImplPointer(std::nullptr_t) : ptr_(nullptr) {} //! Initialize with the given implementation class. explicit PrivateImplPointer(Impl *ptr) : ptr_(ptr) {} //! \cond @@ -147,7 +149,8 @@ class PrivateImplPointer /*! \brief * Sets a new implementation class and destructs the previous one. * - * Needed, e.g., to implement assignable classes. + * Needed, e.g., to implement lazily initializable or copy-assignable + * classes. */ void reset(Impl *ptr) { ptr_.reset(ptr); } //! Access the raw pointer. @@ -156,13 +159,19 @@ class PrivateImplPointer Impl *operator->() { return ptr_.get(); } //! Access the implementation class as with a raw pointer. Impl &operator*() { return *ptr_; } - //! Access the raw pointer. - const Impl *get() const { return ptr_.get(); } //! Access the implementation class as with a raw pointer. const Impl *operator->() const { return ptr_.get(); } //! Access the implementation class as with a raw pointer. const Impl &operator*() const { return *ptr_; } + //! Allows testing whether the implementation is initialized. + explicit operator bool() const { return ptr_ != nullptr; } + + //! Tests for equality (mainly useful against nullptr). + bool operator==(const PrivateImplPointer &other) const { return ptr_ == other.ptr_; } + //! Tests for inequality (mainly useful against nullptr). + bool operator!=(const PrivateImplPointer &other) const { return ptr_ != other.ptr_; } + private: std::unique_ptr ptr_; diff --git a/src/gromacs/utility/datafilefinder.cpp b/src/gromacs/utility/datafilefinder.cpp index e773fff84b..1dcfe1dac8 100644 --- a/src/gromacs/utility/datafilefinder.cpp +++ b/src/gromacs/utility/datafilefinder.cpp @@ -93,7 +93,7 @@ std::string DataFileFinder::Impl::getDefaultPath() */ DataFileFinder::DataFileFinder() - : impl_(NULL) + : impl_(nullptr) { } @@ -142,7 +142,7 @@ std::string DataFileFinder::findFile(const DataFileOptions &options) const { return options.filename_; } - if (impl_.get()) + if (impl_ != nullptr) { std::vector::const_iterator i; for (i = impl_->searchPath_.begin(); i != impl_->searchPath_.end(); ++i) @@ -167,8 +167,8 @@ std::string DataFileFinder::findFile(const DataFileOptions &options) const } if (options.bThrow_) { - const char *const envName = (impl_.get() ? impl_->envName_ : NULL); - const bool bEnvIsSet = (impl_.get() ? impl_->bEnvIsSet_ : false); + const char *const envName = (impl_ != nullptr ? impl_->envName_ : nullptr); + const bool bEnvIsSet = (impl_ != nullptr ? impl_->bEnvIsSet_ : false); std::string message( formatString("Library file '%s' not found", options.filename_)); if (options.bCurrentDir_) @@ -186,7 +186,7 @@ std::string DataFileFinder::findFile(const DataFileOptions &options) const message.append(Path::getWorkingDirectory()); message.append(" (current dir)"); } - if (impl_.get()) + if (impl_ != nullptr) { std::vector::const_iterator i; for (i = impl_->searchPath_.begin(); i != impl_->searchPath_.end(); ++i) @@ -230,7 +230,7 @@ DataFileFinder::enumerateFiles(const DataFileOptions &options) const result.push_back(DataFileInfo(".", *i, false)); } } - if (impl_.get()) + if (impl_ != nullptr) { std::vector::const_iterator j; for (j = impl_->searchPath_.begin(); j != impl_->searchPath_.end(); ++j) diff --git a/src/gromacs/utility/directoryenumerator.cpp b/src/gromacs/utility/directoryenumerator.cpp index f6b34f9aef..96ad1c53cc 100644 --- a/src/gromacs/utility/directoryenumerator.cpp +++ b/src/gromacs/utility/directoryenumerator.cpp @@ -273,7 +273,7 @@ DirectoryEnumerator::enumerateFilesWithExtension( DirectoryEnumerator::DirectoryEnumerator(const char *dirname, bool bThrow) - : impl_(NULL) + : impl_(nullptr) { GMX_RELEASE_ASSERT(dirname != NULL && dirname[0] != '\0', "Attempted to open empty/null directory path"); @@ -281,7 +281,7 @@ DirectoryEnumerator::DirectoryEnumerator(const char *dirname, bool bThrow) } DirectoryEnumerator::DirectoryEnumerator(const std::string &dirname, bool bThrow) - : impl_(NULL) + : impl_(nullptr) { GMX_RELEASE_ASSERT(!dirname.empty(), "Attempted to open empty/null directory path"); @@ -294,7 +294,7 @@ DirectoryEnumerator::~DirectoryEnumerator() bool DirectoryEnumerator::nextFile(std::string *filename) { - if (!impl_.get()) + if (impl_ == nullptr) { filename->clear(); return false; diff --git a/src/gromacs/utility/gmxregex.cpp b/src/gromacs/utility/gmxregex.cpp index 4b3270eca7..87ccafa9b1 100644 --- a/src/gromacs/utility/gmxregex.cpp +++ b/src/gromacs/utility/gmxregex.cpp @@ -177,7 +177,7 @@ class Regex::Impl #endif Regex::Regex() - : impl_(NULL) + : impl_(nullptr) { } @@ -198,7 +198,7 @@ Regex::~Regex() /*! \cond libapi */ bool regexMatch(const char *str, const Regex ®ex) { - if (regex.impl_.get() == NULL) + if (regex.impl_ == nullptr) { return false; } -- 2.11.4.GIT