On 31 July 2012 14:29, Matus Uzak <matus.u...@ixonos.com> wrote: > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/105519/ > > On July 16th, 2012, 5:14 p.m., *Friedrich W. H. Kossebau* wrote: > > I wonder if it would no be a better fix to add a parameter for the default > value to the macro STRING_TO_INT, to which the variable gets set if the > string is empty. Might be more readable code not only to me. > > Only issue would be the usage in > STRING_TO_INT(pitchFamily, pitchFamilyInt, "latin@pitchFamily") > as that needs no default value, given that "if (!pitchFamily.isEmpty()) {" > already before. But then this code is already now not a perfect match for the > macro, as the macro again checks if pitchFamily is empty. > Perhaps that code could be changed from > if (!pitchFamily.isEmpty()) { > int pitchFamilyInt; > STRING_TO_INT(pitchFamily, pitchFamilyInt, "latin@pitchFamily") > to > int pitchFamilyInt; > STRING_TO_INT(pitchFamily, pitchFamilyInt, -1, "latin@pitchFamily") > if (pitchFamilyInt != -1) { > with the new macro. > > Just a proposal. Because you will find out that this patch is not complete, > there are more places where the original variable will get used uninitliazed, > because there is no default value. With the same macro STRING_TO_INT this > seems at least true for > int m_svgWidth; //! set by read_ext() > int m_svgHeight; //! set by read_ext() > int m_svgChX; //!< set by read_chOff() > int m_svgChY; //!< set by read_chOff() > int m_svgChWidth; //! set by read_chExt() > int m_svgChHeight; //! set by read_chExt() > All are never initialized if their string is empty. And as the other > STRING_TO_* macros seem to follow the same pattern, there is even more to do > :) > > On July 23rd, 2012, 10:29 p.m., *Jarosław Staniek* wrote: > > For cases like m_svgChHeight, read_chExt() may be not called at all e.g. for > invalid input -- since m_svgChHeight is in class scope it should be > initialized in the ctor. having initializing in more than one place would be > error prone. > > The idea of these macros is apparently that the destination variables are > initialized (once) anyway, possibly in place where they are defined. > > So I think the macros can stay as they are now. > > Regarding missing initializations: definitely please go on to fix them (my > two findings are from compiler, you apparently have found more). > > There's no need for initialization at this place, because part of the > STRING_TO_INT code is to return immediately if the conversion fails. > > > I think this is not the problem Friedrich tried to address. He meant the case of empty string, i.e. when the 'destination' variable isn't touched by the macro.
But I think my suggestion for initialization it elsewhere (at earlier stage, in a single place) is better. -- regards / pozdrawiam, Jaroslaw Staniek Kexi & Calligra & KDE | http://calligra.org/kexi | http://kde.org Qt Certified Specialist | http://qt-project.org http://www.linkedin.com/in/jstaniek
_______________________________________________ calligra-devel mailing list calligra-devel@kde.org https://mail.kde.org/mailman/listinfo/calligra-devel
