Re: boost::shared_ptr
Le 05/04/2018 à 04:58, Richard Kimberly Heck a écrit : What do we use instead now? I'm trying to update an old patch of Georg's for bug 7474, and it uses boost::shared_ptr. Riki Using "git log -Sscoped_ptr", I find commit ca8709aaf5f5f14aae1978403e13aac3a93506aa Author: Guillaume Munch Date: Thu Jun 2 23:49:36 2016 +0100 Replace boost::scoped_ptr with unique_ptr Note that unique_ptr is not equivalent to scoped_ptr, since it cannot be copied directly. JMarc
boost::shared_ptr
What do we use instead now? I'm trying to update an old patch of Georg's for bug 7474, and it uses boost::shared_ptr. Riki
Re: Killing boost::shared_ptr
Abdelrazak Younes wrote:
rgheck wrote:
The attached patch does as advertised, replacing the shared pointer
with a global cache of sorts of the TextClass's used by Buffers---or,
more strictly, constructed by BufferParams::makeTextClass(). The
action is in TextClass.{h,cpp}.
I've left the typedef in TextClassPtr.h. At the moment, it's kind of
silly. But I've left it mostly because it helps to identify where the
TextClass's stored in the TextClassBundle are used, and maybe it'd be
worth having some sort of strong typedef like the one for
BaseClassIndex here.
Looks like very good work.
And thanks for this, too.
You told me I should do it this way a long time ago. Well, you were right.
rh
Re: Killing boost::shared_ptr
Abdelrazak Younes wrote:
I need to check whether the textClass_ member of InsetCollapsable is
needed now. I think not.
Now that the Buffer is accessible following Andre' recent change, I
think not indeed.
This actually had to do with something else. It got added, I think, by
Bo to fix a crash that occurred during cut and paste, or something of
the sort. The problem was that InsetLayouts are stored in TextClass
objects, and there were times when the boost::shared_ptr would go out of
scope and the TextClass object would be released, and boom. Now that
we're holding these things in a global list, this shouldn't be an issue.
Some style comments below.
Thanks as always.
> +bool
InsetCommandParams::writeEmptyOptional(ParamInfo::const_iterator ci)
const
> +{
> +if (!ci->isOptional())
> +BOOST_ASSERT(false);
> +++ci; // we want to start with the next one
Hum, it's probably better to make a copy here of ci in order to avoid
confusion.
A copy needs to be made here somewhere for sure. I was doing it when the
parameter is passed. But we could do:
+bool
InsetCommandParams::writeEmptyOptional(ParamInfo::const_iterator & it)
const
+{
+if (!it->isOptional())
+BOOST_ASSERT(false);
+ParamInfo::const_iterator ci = it;
+++ci;
instead. But that doesn't seem much clearer.
I'll commit this tonight.
Richard
Re: Killing boost::shared_ptr
rgheck wrote:
The attached patch does as advertised, replacing the shared pointer with
a global cache of sorts of the TextClass's used by Buffers---or, more
strictly, constructed by BufferParams::makeTextClass(). The action is in
TextClass.{h,cpp}.
I've left the typedef in TextClassPtr.h. At the moment, it's kind of
silly. But I've left it mostly because it helps to identify where the
TextClass's stored in the TextClassBundle are used, and maybe it'd be
worth having some sort of strong typedef like the one for BaseClassIndex
here.
Looks like very good work.
I need to check whether the textClass_ member of InsetCollapsable is
needed now. I think not.
Now that the Buffer is accessible following Andre' recent change, I
think not indeed.
Comments welcome.
Some stile comments below.
> Index: src/insets/InsetCommandParams.cpp
> ===
> +bool ParamInfo::ParamData::isOptional() const
> +{
> + return type_ == ParamInfo::LATEX_OPTIONAL ||
> + type_ == ParamInfo::LATEX_KV_OPTIONAL;
> +}
> +
> +
> +bool ParamInfo::ParamData::isKeyValArg() const
> +{
> + return type_ == ParamInfo::LATEX_KV_REQUIRED ||
> + type_ == ParamInfo::LATEX_KV_OPTIONAL;
> +}
> +
> +#if 0
I'd rather have not '#if 0' that are bound to stay forever.
> +//presently unused but perhaps useful
> +bool ParamInfo::ParamData::isArgument() const
> +{
> + return isOptional() || isRequired();
> +}
I think it's better to be explicit in the code and use 'isOptional() ||
isRequired()' instead.
> +bool ParamInfo::ParamData::isRequired() const
> +{
> + return type_ == ParamInfo::LATEX_REQUIRED ||
+ type_ == ParamInfo::LATEX_KV_REQUIRED;
The operator on the second line please:
> + || type_ == ParamInfo::LATEX_KV_REQUIRED;
>
> +}
> +#endif
I guess 'isRequired()' will probably be used soon.
> +// note that this returns FALSE if name corresponds to a `parameter'
> +// of type LATEX_KV_*, because these do not really represent parameters
> +// but just argument places.
> bool ParamInfo::hasParam(std::string const & name) const
Put this comment in the doxygen tag in the header please.
> +ParamInfo::ParamData const &
> + ParamInfo::operator[](std::string const & name) const
> +{
> + BOOST_ASSERT(hasParam(name));
> + const_iterator it = begin();
> + const_iterator last = end();
> + for (; it != last; ++it) {
> + if (it->name() == name)
> + return *it;
>}
> + return *it; // silence warning
> }
In order to get rid of the comment above you can do:
+ for (; it != last; ++it) {
+ if (it->name() == name)
+ break;
}
+ return *it;
> +// returns true if a non-empty optional parameter follows ci
Doxygen comment in header please (use \return).
> +bool
InsetCommandParams::writeEmptyOptional(ParamInfo::const_iterator > ci) const
> +{
> + if (!ci->isOptional())
> + BOOST_ASSERT(false);
> + ++ci; // we want to start with the next one
Hum, it's probably better to make a copy here of ci in order to avoid
confusion.
Killing boost::shared_ptr
The attached patch does as advertised, replacing the shared pointer with
a global cache of sorts of the TextClass's used by Buffers---or, more
strictly, constructed by BufferParams::makeTextClass(). The action is in
TextClass.{h,cpp}.
I've left the typedef in TextClassPtr.h. At the moment, it's kind of
silly. But I've left it mostly because it helps to identify where the
TextClass's stored in the TextClassBundle are used, and maybe it'd be
worth having some sort of strong typedef like the one for BaseClassIndex
here.
I need to check whether the textClass_ member of InsetCollapsable is
needed now. I think not.
Comments welcome.
Richard
Index: src/insets/InsetCommandParams.cpp
===
--- src/insets/InsetCommandParams.cpp (revision 23198)
+++ src/insets/InsetCommandParams.cpp (working copy)
@@ -30,10 +30,10 @@
#include "Lexer.h"
#include "support/debug.h"
+#include "support/docstream.h"
#include "support/ExceptionMessage.h"
#include "support/gettext.h"
#include "support/lstrings.h"
-#include "support/docstream.h"
#include
@@ -42,55 +42,85 @@
namespace lyx {
-ParamInfo::ParamData::ParamData(std::string const & s, bool b) :
- name_(s), optional_(b)
+ParamInfo::ParamData::ParamData(std::string const & s, ParamType t) :
+ name_(s), type_(t)
{}
+bool ParamInfo::ParamData::isOptional() const
+{
+ return type_ == ParamInfo::LATEX_OPTIONAL ||
+ type_ == ParamInfo::LATEX_KV_OPTIONAL;
+}
+
+
+bool ParamInfo::ParamData::isKeyValArg() const
+{
+ return type_ == ParamInfo::LATEX_KV_REQUIRED ||
+ type_ == ParamInfo::LATEX_KV_OPTIONAL;
+}
+
+#if 0
+//presently unused but perhaps useful
+bool ParamInfo::ParamData::isArgument() const
+{
+ return isOptional() || isRequired();
+}
+
+
+bool ParamInfo::ParamData::isRequired() const
+{
+ return type_ == ParamInfo::LATEX_REQUIRED ||
+ type_ == ParamInfo::LATEX_KV_REQUIRED;
+}
+#endif
+
bool ParamInfo::ParamData::operator==(ParamInfo::ParamData const & rhs) const
{
- return name() == rhs.name() && isOptional() == rhs.isOptional();
+ return name() == rhs.name() && type() == rhs.type();
}
+// note that this returns FALSE if name corresponds to a `parameter'
+// of type LATEX_KV_*, because these do not really represent parameters
+// but just argument places.
bool ParamInfo::hasParam(std::string const & name) const
{
const_iterator it = begin();
- for (; it != end(); ++it) {
+ const_iterator last = end();
+ for (; it != last; ++it) {
if (it->name() == name)
- return true;
+ return !it->isKeyValArg();
}
return false;
}
-void ParamInfo::add(std::string const & name, bool opt)
+void ParamInfo::add(std::string const & name, ParamType type)
{
- info_.push_back(ParamData(name, opt));
+ info_.push_back(ParamData(name, type));
}
bool ParamInfo::operator==(ParamInfo const & rhs) const
{
- // the idea here is to check each ParamData for equality
- const_iterator itL = begin();
- const_iterator itR = rhs.begin();
- const_iterator endL = end();
- const_iterator endR = rhs.end();
- while (true) {
- // if they both end together, return true
- if (itL == endL && itR == endR)
-return true;
- // but if one ends before the other, return false
- if (itL == endL || itR == endR)
- return false;
- //check this one for equality
- if (*itL != *itR)
- return false;
- // equal, so check the next one
- ++itL;
- ++itR;
+ if (size() != rhs.size())
+ return false;
+ return equal(begin(), end(), rhs.begin());
+}
+
+
+ParamInfo::ParamData const &
+ ParamInfo::operator[](std::string const & name) const
+{
+ BOOST_ASSERT(hasParam(name));
+ const_iterator it = begin();
+ const_iterator last = end();
+ for (; it != last; ++it) {
+ if (it->name() == name)
+ return *it;
}
+ return *it; // silence warning
}
@@ -318,6 +348,66 @@
}
+docstring InsetCommandParams::makeKeyValArgument() const
+{
+ odocstringstream os;
+ bool didone = false;
+ ParamInfo::const_iterator it = info_.begin();
+ ParamInfo::const_iterator end = info_.end();
+ for (; it != end; ++it) {
+ if (!it->isKey())
+ continue;
+ string const & name = it->name();
+ docstring const & data = (*this)[name];
+ if (data.empty())
+ continue;
+ if (didone)
+ os << ",";
+ else
+ didone = true;
+ os << from_utf8(name) << "=" << data;
+ }
+ return os.str();
+}
+
+
+// returns true if a non-empty optional parameter follows ci
+bool InsetCommandParams::writeEmptyOptional(ParamInfo::const_iterator ci) const
+{
+ if (!ci->isOptional())
+ BOOST_ASSERT(false);
+ ++ci; // we want to start with the next one
+ ParamInfo::const_iterator end = info_.end();
+ for (; ci != end; ++ci) {
+ switch (ci->type()) {
+ case ParamInfo::LATEX_KEY:
+ case ParamInfo::LYX_INTERNAL:
+ break;
+
+ case ParamInfo::LATEX_REQUIRED:
+ case ParamInfo::LATEX_KV_REQUIRED:
+ return false;
+
+ case ParamInfo::LATEX_OPTIONAL: {
+ std::string const & name = ci->name();
+ docstring const
boost::shared_ptr
In my working on the graphics loading facility (src/graphics), I am in a situation where I have GraphicsCacheItem and GraphicsCacheItem_pimpl just for the sake of reference counting the pimpl. I'd like to get rid of this and simply use a reference counting wrapper. I've found boost:shared_ptr in the lyx cvs but it requires exceptions to use it. It seems to me that there is a consensus currently against using exceptions, so the other course is to take the share_ptr, remove the exceptions requiring stuff from it and place it in src/support/ Any objections? -- Baruch Even http://baruch.ev-en.org/
