Am Sonntag, 4. August 2013, 18:48:17 schrieb Jos van den Oever: > > On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote: > > > Meh, too late, committed while I was giving it finally also a look. > > > > Please still pick up the following comments and do a fix-up commit: > Thanks for the thorough look. You caught one bug and quite some style > issues. > > wrt to the product remark, you seem more knowledgable on the subject and i > dont want to learn more cmake folklore. > > Pushing as a fixup. > > > On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote: > > > devtools/rng2cpp/rng2cpp.cpp, line 959 > > > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176045#file176045l > > > ine959>> > > > > isEmpty() > > A constant may be "". > > > On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote: > > > devtools/rng2cpp/rng2cpp.cpp, line 962 > > > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176045#file176045l > > > ine962>> > > > > better isEmpty() than isNull(), unless that difference is important > > > here > > why is it better?
Depends on the usecase of course. Normally an empty string has the same meaning like a null string. But people might not be aware and just pass a "", which will fail the test on isNull(). So isEmpty() catches all of that. But if here isNull() (as in: no string at all) has a separate meaning, because a "" is valid option, than of course this "is better" does not apply at all. Was not obvious to me here that "" is a valid value :) > > On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote: > > > devtools/CMakeLists.txt, line 10 > > > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176043#file176043l > > > ine10>> > > > > Please turn rng2cpp into a product. > > > Means you add a product definition for this tool and wrap the call > > > add_subdirectory(rng2cpp) into the proper conditions. > If you think this is important, you can do so, but I do not see the benefit. > The dependencies are set correctly. These are not dependencies as in make terms, but in "what to enable for build" terms. So if somebody just wants to build Krita & Words, it is possible to just name those two in that custom productset, and still all internal required dependencies and nice-to-have addons are automatically added to be build as well, but not more. Cared for that, okay. > > On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote: > > > libs/odf/writeodf/odfwriter.h, lines 67-70 > > > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176058#file176058l > > > ine67>> > > > > This method can result in data-loss, no? > > > > > > Passing quint64 value to QString::number(long) will not work on > > > 32-bit systems, as long is only 32bit there. And possibly on Win64 > > > as well IIRC. (Also long is signed). > > > > > > So the API of this method promises more then it can. > > > > > > Please fix! > > on 32-bit qlonglong would be automatically used. ah, ignore me here indeed, somehow I missed the other integer-like overloads of QString::number(x) :/ > > On Aug. 4, 2013, 5:57 p.m., Friedrich W. H. Kossebau wrote: > > > libs/odf/writeodf/odfwriter.h, line 96 > > > <http://git.reviewboard.kde.org/r/111773/diff/5/?file=176058#file176058l > > > ine96>> > > > > so T is always just passed by value? what kind of Ts are expected > > > here? > > T is a pointer for both uses. > void addCompleteElement(const char* cstr); > void addCompleteElement(QIODevice* dev); Ah, right, is indirectly limited by what overloads there are with KoXMLWriter::addCompleteElement(...), okay. Just surprised to see this solved by a templated method, instead of two implemented methods then. Cheers Friedrich _______________________________________________ calligra-devel mailing list calligra-devel@kde.org https://mail.kde.org/mailman/listinfo/calligra-devel
