<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/105519/">http://git.reviewboard.kde.org/r/105519/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 16th, 2012, 5:14 p.m., <b>Friedrich W. H. Kossebau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 :)
</pre>
</blockquote>
<p>On July 23rd, 2012, 10:29 p.m., <b>Jarosław Staniek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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).
</pre>
</blockquote>
<p>On July 31st, 2012, 12:30 p.m., <b>Matus Uzak</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">{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.</pre>
<br />
<p>- Matus</p>
<br />
<p>On July 11th, 2012, 9:43 p.m., Jarosław Staniek wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Calligra and Matus Uzak.</div>
<div>By Jarosław Staniek.</div>
<p style="color: grey;"><i>Updated July 11, 2012, 9:43 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"> </pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>filters/libmsooxml/MsooXmlCommonReaderDrawingMLImpl.h <span style="color: grey">(492f2ea7f6f01a0893d3f7a26b7cc2b36e82d49e)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105519/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>