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 _______________________________________________ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development