On Mon, Dec 10, 2007 at 11:02:22PM +0100, Stefan Schimanski wrote:
> Jean-Marc Lasgouttes schrieb:
>> Stefan Schimanski <[EMAIL PROTECTED]> writes:
>>
>>
>>> For insets you are right, I assume they open an environment, which is
>>> not correct in general. Have to check how to do this properly. The
>>> InsetBranch is on my list anyway. Haven't looked into that.
>>>
>>
>> The best is probably a virtual method for insets telling whether there
>> is a scope.
> Added isMacroScope() to Inset as virtual method, defaulting to "true".
>
> I added an implementation to InsetBranch which sets the scope only if the
> branch is deselected. So the macros follow the Branch semantics now.
>
> Stefan
>
[...]
> ///
> class MacroData {
> public:
> - ///
> + /// Constructor to make STL containers happy
> MacroData();
> + /// Create lazy MacroData which only queries the macro template when
> needed
> + MacroData(Buffer const & buf, DocIterator const & pos);
This works?
> + /// Create non-lazy MacroData which directly queries the macro template
> + MacroData(MathMacroTemplate const & macro);
> +
> ///
> - MacroData(docstring const & definition,
> - std::vector<docstring> const & defaults, int nargs, int
> optionals,
> - docstring const & display, std::string const & requires);
> + virtual ~MacroData() {}
Do we ever derive from MacroData? If not, why 'virtual'?
> -void MathData::collectOptionalParameters(Cursor & cur,
> +void MathData::collectOptionalParameters(Cursor * cur,
> const size_type numOptionalParams, std::vector<MathData> & params,
> size_t & pos, const pos_type macroPos, const int thisPos, const int
> thisSlice)
> {
> @@ -614,7 +626,7 @@
> // is a [] block following which could be an optional parameter?
> if (operator[](pos)->getChar() != '[')
> break;
> -
> +
_no_ trailing whitespace would be even nicer ;-)
> [...]
> -MacroData::MacroData(docstring const & definition,
> - std::vector<docstring> const & defaults,
> - int numargs, int optionals, docstring const & display,
> - string const & requires)
> - : definition_(definition), numargs_(numargs), display_(display),
> - requires_(requires), lockCount_(0), redefinition_(false),
> - optionals_(optionals), defaults_(defaults)
> +
> +
> +MacroData::MacroData(Buffer const & buf, DocIterator const & pos)
> + : buffer_(&buf), pos_(pos), queried_(false), numargs_(0),
> + optionals_(0), lockCount_(0), redefinition_(false),
> + type_(MacroTypeNewcommand)
> {
> - defaults_.resize(optionals);
> }
This seems to rely for a certain time on the validity of the passed and
stored DocIterator. We have almost no guarantee for that, though.
Could you please elaborate a bit on what assumptions are made here and
why they will be fulfilled?
[...]
> +void MacroData::queryData(MathMacroTemplate const & macro) const
> +{
> + if (!queried_) {
> + queried_ = true;
> + definition_ = macro.definition();
> + numargs_ = macro.numArgs();
> + display_ = macro.displayDefinition();
> + redefinition_ = macro.redefinition();
> + type_ = macro.type();
> + optionals_ = macro.numOptionals();
> + macro.getDefaults(defaults_);
> + }
> +}
Early return please. (More occurences below)
> +void MacroData::updateData() const
> +{
> + if (!queried_) {
> + BOOST_ASSERT(buffer_ != 0);
> +
> + // Try to fix position DocIterator. Should not do anything in
> theory.
> -MacroData const & MacroTable::get(docstring const & name) const
> -{
> const_iterator it = find(name);
> - BOOST_ASSERT(it != end());
> - return it->second;
> + if (it == end())
> + return 0;
> + else
> + return &it->second;
I find
return it == end() ? 0 : &it->second;
less verbose and clearer.
> - /// macro table
> - typedef std::map<unsigned int, MacroData, std::greater<int> >
> PositionToMacroMap;
> - typedef std::map<docstring, PositionToMacroMap> NameToPositionMacroMap;
> - NameToPositionMacroMap macros;
> + /// macro tables
> + typedef std::pair<DocIterator, MacroData> ScopeMacro;
> + typedef std::map<DocIterator, ScopeMacro, std::greater<DocIterator> >
> PositionScopeMacroMap;
Does this 'inverted logic' make things much clearer in the
implementation? If not (and I don't see it...) I'd think it'd
be better to stick with the default...
[...]
> + // find macros in included files
> + Impl::PositionScopeBufferMap::iterator it
> + = d->position_to_children.upper_bound(pos);
> + while (it != d->position_to_children.end()) {
> + // do we know something better (i.e. later) already?
> + if (it->first < bestPos )
> + break;
> +
> + // scope ends behind pos?
> + if (pos < it->second.first) {
> + // look for macro in external file
> + d->macro_lock = true;
> + MacroData const * data
> + = it->second.second->getMacro(name, false);
> + d->macro_lock = false;
> + if (data) {
> + bestPos = it->first;
> + bestData = data;
> + break;
> + }
> + }
> +
> + // External file out of scope, try the previous
> + // NOTE: the map is sorted backwards,
> + // hence it's a logical it--
> + it++;
Especially if it makes explanations necessary ;-)
Apart from that: Patch _looks_ nice and I have no clue whether it works
;-}
Andre'
