| habacker |
Throwing exceptions was not an elegant solution here. Returning an
invalid date and having errorCode and errorMessage methods to provide
information about the last conversion seems to be a better solution.
In the process, I also cleaned up the interface a bit and hide
implementation details behind a d-pointer.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
| src/alkdateformat.cpp | ||
|---|---|---|
| 110 | This is unrelated, but should this code not used also for Qt >= 5.0 and < 5.8 | |
| 428 | Please use 4 spaces as indention | |
| 433–436 | dito. We already used kde-dev-scripts/uncrustify-kf5 to use a common format. Seems to be good time to rerun this script on master branch as a single commit and refresh this review with a crustified update. | |
| src/alkdateformat.h | ||
| 67–68 | Why are you not using this class Private; Private *const d; as already used by other classes ? We should use the same style in all classes, either this, if it has advantages or what has been already used. | |
| src/alkdateformat.cpp | ||
|---|---|---|
| 117 | The Qt5 equivalent has been changed to QDate convertStringKMyMoney(const QString& _in, bool _strict, unsigned _centurymidpoint) without const. Please keep this method in sync. | |
| src/alkdateformat.h | ||
| 67–68 | Using AlkDateFormat::ErrorCode AlkDateFormat::lastError() const
{
Q_D(const AlkDateFormat);
return d->m_errorCode;
}adds a redundant definition to each public method with additional runtime costs (casts) without any benefit, where is would be otherwise simply AlkDateFormat::ErrorCode AlkDateFormat::lastError() const
{
return d->m_errorCode;
}Please use class Private; | |
| src/alkdateformat.cpp | ||
|---|---|---|
| 110 | I see, thanks for this pointer. A related comment at this place explaining this would be nice. | |
| src/alkdateformat.h | ||
|---|---|---|
| 67–68 |
You underestimate modern compilers/optimizers significantly: 0000000000000190 <_ZNK13AlkDateFormat9lastErrorEv>: 190: 48 8b 07 mov (%rdi),%rax 193: 8b 40 08 mov 0x8(%rax),%eax 196: c3 retq I don't see any runtime overhead in here. | |
I'm going to submit this with removed obsolete forward declaration
class AlkDateFormatPrivate;
| src/alkdateformat.h | ||
|---|---|---|
| 67–68 | Indeed that looks good :-) I looked at the macro, which looked not trivial #define Q_DECLARE_PRIVATE(Class)\
inline Class##Private* d_func() {\
return reinterpret_cast<Class##Private*>(qGetPtrHelper(d_ptr));\
}\
inline const Class##Private d_func() const {\
return reinterpret_cast<const Class##Private *>(qGetPtrHelper(d_ptr));\
}\
friend class Class##Private;The complexitivity of the Qt way is required to handle all needs from the huge Qt library including derived private classes, which I do not see here yet. If required, we can add this without much work to all classes probably by a gawk or sed script in future, so nothing is lost. :-) | |