<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 24, 2015 at 8:08 PM, Amos Jeffries <span dir="ltr"><<a href="mailto:squid3@treenet.co.nz" target="_blank">squid3@treenet.co.nz</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 25/08/2015 4:49 a.m., Kinkie wrote:<br>
> Hi,<br>
>    this is the first of a series of patches stemming from the<br>
> coverity-fixes effort.<br>
> The aim of this first submission  is to implement WholeEnum and EnumRange<br>
> and supporting unit tests.<br>
><br>
> Big-context patch attached<br>
><br>
<br>
</span>in <a href="http://configure.ac" rel="noreferrer" target="_blank">configure.ac</a>:<br>
* no need for two lines, just add the macro straight after the list of<br>
other types.<br>
<br></blockquote><div>Ok.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
in src/Makefile.am:<br>
* since this is scoped at the enum operator please remove the<br>
nodist_tests_testX_SOURCES change<br>
 - I will pick that up in the Makefile.am polishing later<br></blockquote><div><br></div><div>Ok<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
* please do omit tests_testEnumIterator_DEPENDENCIES<br>
 - all it does now is harm repeatability of your test results<br>
<br></blockquote><div>Ok<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
in src/base/EnumIterator.h:<br>
* the description of requirements for the marker on WholeEnum is a<br>
little bit mind-boggling.<br>
 - a bullet list might be better here if you can find the doxygen syntax<br>
for that. IIRC it was ' - ', but double check if it needs empty lines<br>
between.<br></blockquote><div><br></div><div>Done. Yes it's a minus, no it doesn't need empty lines.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
* the iterator_type being used in static_cast before the typedef _looks_<br>
very dangerous (even if its fine and works).<br>
 - I would place typedef up the top in their own protected area to avoid<br>
a) depending on implicit compiler behaviour and b) future developer<br>
mistakes/misunderstanding.<br></blockquote><div><br></div><div>Ok. Done.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I dont see the first-user code that we normally require for accepting<br>
these. I know its coming with a rather big followup, just would be good<br>
if there was a simple use that could be used as exemplar.<br></blockquote><div><br></div><div>as icp_opcode. Instrumentation needed for iterators has been implemented alongside the existing ICP_MAX value which is apparently (and unfortunately) loaded with more than one meaning.<br><br></div><div>I caught a limitation in mk-string-arrays which looks for "typedef" instead of "enum" to detect the start of the symbol definitions; fixed that adding support for "enum enumname" and "enum class enumname" enumBegin_ and enumEnd_ are not caught because the script requires the first letter of the enum symbol to be capitalized.<br><br></div><div>v2 patch attached.<br><br></div><div>Thanks!<br></div></div><br>-- <br><div class="gmail_signature">    Francesco</div>
</div></div>