[Development] Calendar Systems proposal

[Thiago Macieira]

On Tuesday, 8 August 2017 08:49:36 PDT Edward Welbourne wrote:
> Thiago Macieira (7 August 2017 06:14)
> 
> > ... there are bigger problems with the implementation, starting with
> > QAbstractCalendar having a static protected QMap member.
> 
> That's my fault.  We're going to need some way for QML/V4 to get at
> calendars; and I want to ensure our design leaves scope for client code
[cut]
> 
> Please suggest a better solution (and explain the problem with the
> present solution), either here or in the review.

The problem is the presence of a protected, static, non-POD member. Remove it.

Store the data in a Q_GLOBAL_STATIC in qabstractcalendar.cpp. If you need to 
get a listing or do any kind of searching, add static functions to 
QAbstractCalendar. See QTextCodec for inspiration.

> > The big problem is how you've implemented the new API in QDateTime and
> > QLocale. There's code duplication that cannot be there in QLocale,
> 
> That's probably best addressed by you commenting on the review; I'm not
> sure what duplication you're referring to ("cannot be there" is strong
> language), although I do know about dateTimeToString().  There are a few
> places I expect to find myself doing clean-up in the aftermath of
> getting this all in, but I don't mind doing some of it before-hand.

The big code block that was added in the commit to qlocale.cpp looks like a 
copy & paste of existing code.

> Note, also, that this moves calendar-related data out of QLocale's
> CLDR-derived data blob into calendar-specific data blobs - a step in the
> general direction of making QLocale less monolithic.

That's good.

> > but the way you've removed the duplication in QDateTime also needs
> > changing for performance reasons.
> > 
> > int QDate::year() const
> > {
> >     return year(QGregorianCalendar());
> > }
> > 
> > This creates a polymorphic object and makes a call that ends up delegating
> > to it in
> > 
> >             if (cal.julianDayToDate(jd, y, m, d))
> 
> Please elaborate: why is this a problem ?  The Gregorian arithmetic
> naturally belongs in a calendar implementation.  The date-time code
> should naturally call it, rather than duplicating it in static functions
> of its own (which have, naturally, been moved to become its methods).

The problems are:
 - creating a polymorphic type (QGregorianCalendar) [performance]
 - passing it by reference instead of a pointer [coding style]
 - calling a non-inline function to do the calculation [performance]

The performance problems are due to performance regressions. I'd rather you 
inverted the deduplication: let QDate have the calculation and call that from 
QGregorianCalendar.

> > The commit also includes changes that look like unrelated clean-ups and
> > will need to be split into different commits.
> 
> Please point these out on the review.  Some of them might not be as
> unrelated as you think - I did a fair bit of pulling separable changes
> out already, rebasing Soroush's work onto the results.  I may well have
> missed some, but there were bits that couldn't be separated.

I will.

> > It's at this point lacking documentation
> 
> Indeed, there remains work to be done.  On the other hand, deciding what
> shape the API should be is worth doing before taking pains over
> documenting every detail - that would all change if we decide we need to
> change the API.

Sure. I just replied here because it looked like it was a request to make it 
before the FF.

> 
> > and there are a couple of coding style mistakes.
> 
> Please note in Gerrit.

	s/\w& / &/g

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center