On Saturday, 5 August 2017 05:08:23 PDT Soroush Rabiei wrote: > I believe our proposed change (containing locale backend , date and time > classes and related widgets) is ready to be merged (maybe after some minor > improvements) > > I would like to ask someone to review this change: > > https://codereview.qt-project.org/#/c/182341/
Ok, a quick review's quick conclusions: I mostly like your API. It looks simple and well integrated into QDateTime and QLocale. There are a couple of minor problems in the API itself, like QCalendarWidget::calendar() returning a reference. That's just wrong and needs work. But there are bigger problems with the implementation, starting with QAbstractCalendar having a static protected QMap member. 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, 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)) The commit also includes changes that look like unrelated clean-ups and will need to be split into different commits. It's at this point lacking documentation and there are a couple of coding style mistakes. In all, good work and I like how this is coming along, but it's not "ready for beta" right now, so it shouldn't be included in dev until after 5.11 opens for new features. -- 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