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.
