Hi Richard, I've got some questions about this.
> Author: rgheck > Date: Wed Jun 8 02:12:52 2011 > New Revision: 38980 > URL: http://www.lyx.org/trac/changeset/38980 > > Log: > Fix problem with static error list. > > It's amazing we haven't seen problems with this before. So, the first question is, how come this wasn't a problem then ? > The basic problem is that buf.errorList("whatever") would always return the > same global, static error list, if it did not already exist. So, to a > significant extent, there was only one global error list! > > Modified: > lyx-devel/trunk/src/Buffer.cpp > lyx-devel/trunk/src/Buffer.h > lyx-devel/trunk/src/frontends/qt4/GuiView.cpp > lyx-devel/trunk/src/insets/InsetInclude.cpp > > Modified: lyx-devel/trunk/src/Buffer.cpp > > ============================================================================== > --- lyx-devel/trunk/src/Buffer.cpp Wed Jun 8 01:52:22 2011 > (r38979) > +++ lyx-devel/trunk/src/Buffer.cpp Wed Jun 8 02:12:52 2011 > (r38980) > @@ -3154,9 +3154,9 @@ > } > > > -ErrorList & Buffer::errorList(string const & type) const > +ErrorList const & Buffer::errorList(string const & type) const > { > - static ErrorList emptyErrorList; > + static const ErrorList emptyErrorList; > map<string, ErrorList>::iterator it = d->errorLists.find(type); > if (it == d->errorLists.end()) > return emptyErrorList; > @@ -3165,6 +3165,12 @@ > } > > > +ErrorList & Buffer::errorList(string const & type) > +{ > + return d->errorLists[type]; > +} > + > I don't feel comfortable having these two functions that do not differ in constness only, but also in behavior. I'm afraid this will be confusing in future times. > > > Modified: lyx-devel/trunk/src/Buffer.h > > ============================================================================== > --- lyx-devel/trunk/src/Buffer.h Wed Jun 8 01:52:22 2011 > (r38979) > +++ lyx-devel/trunk/src/Buffer.h Wed Jun 8 02:12:52 2011 > (r38980) > @@ -562,7 +562,8 @@ > /// errors (like parsing or LateX compilation). This method is const > /// because modifying the returned ErrorList does not touch the > document > /// contents. > - ErrorList & errorList(std::string const & type) const; > + ErrorList & errorList(std::string const & type); > + ErrorList const & errorList(std::string const & type) const; > To increase the confusion, the comment above is not in sync with the function declarations themselves. > > > Modified: lyx-devel/trunk/src/frontends/qt4/GuiView.cpp > > ============================================================================== > --- lyx-devel/trunk/src/frontends/qt4/GuiView.cpp Wed Jun 8 01:52:22 > 2011 (r38979) > +++ lyx-devel/trunk/src/frontends/qt4/GuiView.cpp Wed Jun 8 02:12:52 > 2011 (r38980) > @@ -1451,13 +1451,13 @@ > #if EXPORT_in_THREAD && (QT_VERSION >= 0x040400) > // We are called with from_master == false by default, so we > // have to figure out whether that is the case or not. > - ErrorList & el = bv->buffer().errorList(error_type); > + ErrorList & el = const_cast<ErrorList > &>(bv->buffer().errorList(error_type)); > You created two functions, a const one and a non-const one, and then suddenly you need a const_cast which wasn't necessary before. This feels awkward. > if (el.empty()) { > el = bv->buffer().masterBuffer()->errorList(error_type); > from_master = true; > } > #else > Now you're copying the masterBuffer's errorList into the buffer's errorList. The next time you arrive at this code, you will never have an empty el, which means that from_master is set to false, while we actually use the masterBuffer's errorList. > - ErrorList & el = from_master ? > + ErrorList const & el = from_master ? > bv->buffer().masterBuffer()->errorList(error_type) : > bv->buffer().errorList(error_type); > #endif > Does it make sense that in the EXPORT_IN_THREAD case, el is a non-const reference, and in the other case it is a const reference. This seems to be confusing as well in the rest of the code. P.S. You already backported this to branch. I'd prefer if you were a bit more conservative when considering non-trivial changes like this and at least give us an opportunity to comment. Vincent
