> On July 16, 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 :)
> >
>
> 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).
>
>
> Matus Uzak wrote:
> 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.
{m_svgWidth, m_svgHeight} are initialized in the corresponding filter. To
initialize {m_svgChX, m_svgChY, m_svgChWidth, m_svgChHeight} is on my TODO list
and I could resolve it this week.
- Matus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105519/#review15973
-----------------------------------------------------------
On July 11, 2012, 9:43 p.m., Jarosław Staniek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105519/
> -----------------------------------------------------------
>
> (Updated July 11, 2012, 9:43 p.m.)
>
>
> Review request for Calligra and Matus Uzak.
>
>
> Description
> -------
>
> Fixed '<value> may be used uninitialized' warnings
>
> In member function KoFilter::ConversionStatus
> XlsxXmlWorksheetReader::read_spcPct(): lineSpace may be used uninitialized in
> this function
> In member function KoFilter::ConversionStatus
> XlsxXmlWorksheetReader::read_spcPts(): margin may be used uninitialized in
> this function
>
>
> Diffs
> -----
>
> filters/libmsooxml/MsooXmlCommonReaderDrawingMLImpl.h
> 492f2ea7f6f01a0893d3f7a26b7cc2b36e82d49e
>
> Diff: http://git.reviewboard.kde.org/r/105519/diff/
>
>
> Testing
> -------
>
>
>
>
> Thanks,
>
> Jarosław Staniek
>
>
_______________________________________________
calligra-devel mailing list
calligra-devel@kde.org
https://mail.kde.org/mailman/listinfo/calligra-devel
