<div dir="ltr">The MR is now complete, it covers vtkPoints and the vtkDataArray classes, along with vtkStringArray.<div><a href="https://gitlab.kitware.com/vtk/vtk/merge_requests/3176" target="_blank" style="font-size:12.8px">https://gitlab.kitware.com/<wbr>vtk/vtk/merge_requests/3176</a></div><div><br></div><div><a href="https://gitlab.kitware.com/vtk/vtk/merge_requests/3176" target="_blank" style="font-size:12.8px"></a>Adding a VTK_EXPECTS(condition) check to a VTK method causes a ValueError exception to be raised if the condition is not satisfied when the method is called. There was some discussion on the MR about whether a failed index check (e.g. for GetValue(id)) should raise IndexError instead of ValueError. Since VTK_EXPECTS() is just a generic check on the parameter values, I've stuck with ValueError so far. In addition to checking index values, VTK_EXPECTS() is also used to check for NULL pointers in calls where they aren't permitted.</div><div><br></div><div>Any comments are welcome.</div><div><br></div><div>Cheers,</div><div> - David</div><div><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Aug 20, 2017 at 11:04 PM, David Gobbi <span dir="ltr"><<a href="mailto:david.gobbi@gmail.com" target="_blank">david.gobbi@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">I've put together an MR that uses the VTK_EXPECTS() macro to add Python safety checks to vtkPoints:<div><a href="https://gitlab.kitware.com/vtk/vtk/merge_requests/3176" target="_blank">https://gitlab.kitware.com/<wbr>vtk/vtk/merge_requests/3176</a><br></div><div><br></div><div>If the point Id for GetPoint() or SetPoint() is out of range, then Python will raise an exception instead of crashing.</div><span class="gmail-HOEnZb"><font color="#888888"><div><br></div><div> - David</div><div><br></div></font></span></div><div class="gmail-HOEnZb"><div class="gmail-h5"><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 10, 2017 at 12:44 AM, Elvis Stansvik <span dir="ltr"><<a href="mailto:elvis.stansvik@orexplore.com" target="_blank">elvis.stansvik@orexplore.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><span><div><div class="gmail_extra"><div class="gmail_quote">Den 9 aug. 2017 10:54 em skrev "David Gobbi" <<a href="mailto:david.gobbi@gmail.com" target="_blank">david.gobbi@gmail.com</a>>:<br type="attribution"><blockquote class="gmail-m_-5511360773987672582m_6755743777591862714quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Here's what I'm currently thinking about for wrapper hints, please let me know if the syntax is too obtrusive.</div><div><br></div><div>The basic precondition contract, similar to proposed C++ contracts:</div><div><br></div><div> float GetValue(vtkIdType i) VTK_EXPECTS(i >= 0 && i < GetNumberOfValues());</div></div></blockquote></div></div></div><div dir="auto"><br></div></span><div dir="auto">This is really just a stylistic nitpick, and a matter of taste of course, so take it or leave it, but I like it when the var is in between the bounds, e.g.</div><div dir="auto"><br></div><div dir="auto"> 0 <= i && i < <span style="font-family:sans-serif">GetNumberOfValues()</span></div><div dir="auto"><span style="font-family:sans-serif"><br></span></div><div dir="auto"><span style="font-family:sans-serif">As for the hint syntax, I also think what you last posted is readable enough.</span></div><span class="gmail-m_-5511360773987672582HOEnZb"><font color="#888888"><div dir="auto"><span style="font-family:sans-serif"><br></span></div><div dir="auto"><span style="font-family:sans-serif">Elvis</span></div><div dir="auto"><span style="font-family:sans-serif"><br></span></div></font></span><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail-m_-5511360773987672582m_6755743777591862714quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-m_-5511360773987672582h5"><div dir="ltr"><div> </div><div>And here's the format that I'm currently thinking about for wrapper size hints (to replace our "hints" file):</div><div><br></div><div> double *GetPoint(vtkIdType i) VTK_SIZEHINT(return[3]);<br></div><div><br></div><div>When used in combination things start to look ugly, but the method signature is still readable:</div><div><br></div> void SetTuple(vtkIdType i, float *a)<div> VTK_EXPECTS(i >= 0 && i < GetNumberOfTuples())</div><div> VTK_SIZEHINT(a[GetNumberOfComp<wbr>onents()]);<br><div class="gmail_extra"><br></div><div class="gmail_extra"> float *GetTuple(vtkIdType i)<div> VTK_EXPECTS(i >= 0 && i < GetNumberOfTuples())</div><div> VTK_SIZEHINT(return[GetNumberO<wbr>fComponents()]);</div><div><br></div><div>Any thoughts? I'd really like to express the size hint as some kind of contract, but C++ doesn't provide a way to check the number of values pointed to by a pointer.</div><div><br></div><div>As an aside: for vtkDataArray, I don't actually intend to add VTK_SIZEHINT as shown above. Currently the size hints for vtkDataArray are hard-coded into the wrappers, and I'll probably keep them that way in order to keep the C++ headers clean.</div><font color="#888888"><div><br></div><div> - David</div><div><br></div></font></div><div class="gmail-m_-5511360773987672582m_6755743777591862714elided-text"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 7, 2017 at 6:56 AM, David Gobbi <span dir="ltr"><<a href="mailto:david.gobbi@gmail.com" target="_blank">david.gobbi@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi Andras,<div><br></div><div>I agree that these contracts are a better choice than inventing something of our own, I'm going to play around with this to see how easy it would be to add them to the wrappers.</div><div><br></div><div>The macros can be made a little more compact:</div><div><span style="font-family:"Courier New"">T& operator[](size_t i) VTK_PRECONDITION(i >= 0 && i < size());</span><br></div><div><br></div><div>Or we could use <span style="font-family:"Courier New"">VTK_EXPECTS(i >= 0 && i < size());</span></div><div><br></div><div> - David<br></div><div><div class="gmail-m_-5511360773987672582m_6755743777591862714m_-6004200201924893050gmail-m_6204826104124305112gmail-h5"><div><br></div><div><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 4, 2017 at 7:39 PM, Andras Lasso <span dir="ltr"><<a href="mailto:lasso@queensu.ca" target="_blank">lasso@queensu.ca</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div lang="EN-US">
<div class="gmail-m_-5511360773987672582m_6755743777591862714m_-6004200201924893050gmail-m_6204826104124305112gmail-m_1450706126456948411gmail-m_8165851147542036691WordSection1">
<p class="MsoNormal">Eventually, C++17 standard contracts may be used to specify these bounds checks. For now, we may use the same format but add them as comments or protected by macros; and when we switch to C++17 then these can be converted to actual contracts.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Proposal for contracts: <a href="http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2015/n4415.pdf" target="_blank">
http://www.open-std.org/JTC1/S<wbr>C22/WG21/docs/papers/2015/n441<wbr>5.pdf</a><u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Example contract:<u></u><u></u></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">T& operator[](size_t i) [[expects: i >= 0 && i < size()]];<u></u><u></u></span></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">For now, we could do this in VTK:<u></u><u></u></p>
<p class="MsoNormal"><span style="font-family:"Courier New"">T& operator[](size_t i) VTK_CONTRACT([[expects: i >= 0 && i < size()]]);<u></u><u></u></span></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Andras<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal"><a name="m_-5511360773987672582_m_6755743777591862714_m_-6004200201924893050_m_6204826104124305112_m_1450706126456948411_m_8165851147542036691__MailEndCompose"><u></u> <u></u></a></p>
<span></span>
<p class="MsoNormal"><span class="gmail-m_-5511360773987672582m_6755743777591862714m_-6004200201924893050gmail-m_6204826104124305112gmail-m_1450706126456948411gmail-"><b>From:</b> David Gobbi [mailto:<a href="mailto:david.gobbi@gmail.com" target="_blank">david.gobbi@gmail.com</a>] <br>
</span><b>Sent:</b> Friday, August 4, 2017 4:05 PM<br>
<b>To:</b> Berk Geveci <<a href="mailto:berk.geveci@kitware.com" target="_blank">berk.geveci@kitware.com</a>><br>
<b>Cc:</b> Andras Lasso <<a href="mailto:lasso@queensu.ca" target="_blank">lasso@queensu.ca</a>>; VTK Developers <<a href="mailto:vtk-developers@vtk.org" target="_blank">vtk-developers@vtk.org</a>><br>
<b>Subject:</b> Re: [vtk-developers] Safer item access in Python wrapped VTK arrays<u></u><u></u></p><div><div class="gmail-m_-5511360773987672582m_6755743777591862714m_-6004200201924893050gmail-m_6204826104124305112gmail-m_1450706126456948411gmail-h5">
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">For a while, we kicked around the idea of having the hints in the comments. This would be less intrusive than adding C++11 attributes the declarations (i.e. what I've been doing so far).<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">The C++11 attributes are definitely the cleanest way for the wrappers to store the hints, since the hints become part of the parse tree, so I want to keep them as the primary hinting mechanism. Comment-based hints could be added as a secondary
mechanism, i.e. after a declaration has been parsed we can check the docstring for extra hints, and then add the hints to the parse tree before the wrapper code is generated.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">I'll have to search through the archives to remember what kind of syntax we were considering for comment-based hints.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal"> - David<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Fri, Aug 4, 2017 at 1:30 PM, Berk Geveci <<a href="mailto:berk.geveci@kitware.com" target="_blank">berk.geveci@kitware.com</a>> wrote:<u></u><u></u></p>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0cm 0cm 0cm 6pt;margin-left:4.8pt;margin-right:0cm">
<div>
<p class="MsoNormal">As much as I support the concept of range checking in the wrappers, this hint will make things look very ugly and non-standard at a time where we are trying to move towards more standard looking C++. Isn't there a way of doing this without
mangling the signatures?<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div>
<div>
<p class="MsoNormal">On Fri, Aug 4, 2017 at 1:06 PM, David Gobbi <<a href="mailto:david.gobbi@gmail.com" target="_blank">david.gobbi@gmail.com</a>> wrote:<u></u><u></u></p>
</div>
</div>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0cm 0cm 0cm 6pt;margin-left:4.8pt;margin-right:0cm">
<div>
<div>
<div>
<p class="MsoNormal">Hi Andras,<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Yes, this would be possible. It could be hard-coded into the wrappers, or even better, in the header file a generic hint could be added to the index parameter so that the index would be checked against the size of the array:<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal"> float GetValue(vtkIdType idx VTK_RANGECHECK(0,GetNumberOfVa<wbr>lues()));<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">I have a list of wrapper hints at <a href="https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.vtk.org%2FWiki%2FVTK%2FWrapping_hints&data=02%7C01%7Classo%40queensu.ca%7C37f3749bcbbf4422f0ba08d4db741f07%7Cd61ecb3b38b142d582c4efb2838b925c%7C1%7C0%7C636374739163412038&sdata=FtQM8JHWrl8ReFwC3EFD4j0COBmWqm1r6k9b%2BF5tZzU%3D&reserved=0" target="_blank">http://www.vtk.org/Wiki/VTK<wbr>/Wrapping_hints</a>,
and I can add this "RANGECHECK" hint to the "Proposed hints" section.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">I'm hoping to find time to implement more of these wrapper hints over the next few weeks.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><span style="color:rgb(136,136,136)"><u></u> <u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="color:rgb(136,136,136)"> - David<u></u><u></u></span></p>
</div>
<div>
<div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Fri, Aug 4, 2017 at 9:53 AM, Andras Lasso <<a href="mailto:lasso@queensu.ca" target="_blank">lasso@queensu.ca</a>> wrote:<u></u><u></u></p>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0cm 0cm 0cm 6pt;margin-left:4.8pt;margin-right:0cm">
<div>
<div>
<p class="MsoNormal"><span style="color:rgb(31,73,125)">Hi David,</span><u></u><u></u></p>
<p class="MsoNormal"><span style="color:rgb(31,73,125)"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="color:rgb(31,73,125)">Users who mostly use VTK via Python wrapping are sometimes surprised how easily they can crash the application by accessing out-of-bounds elements in
arrays. For example, this crashes an application:</span><u></u><u></u></p>
<p class="MsoNormal"><span style="color:rgb(31,73,125)"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:8.5pt;font-family:Courier;color:black">>>>
</span><span style="font-size:8.5pt;font-family:Courier;color:rgb(0,0,150)">a=vtk.vtkStringArray()</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:8.5pt;font-family:Courier;color:black">>>>
</span><span style="font-size:8.5pt;font-family:Courier;color:rgb(0,0,150)">print(a.GetValue(0))</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:8.5pt;font-family:Courier;color:rgb(0,0,150)"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="color:rgb(31,73,125)">I first thought that it should be handled by documentation and training, but it would be nicer if the wrappers could handle this.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="color:rgb(31,73,125)"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="color:rgb(31,73,125)">Would it be feasible to add bounds check to VTK array accessors in Python wrappers, so that in case of an out of bounds access, a Python exception would
be raised instead of crashing the application?</span><u></u><u></u></p>
<div>
<div>
<div>
<div>
<p class="MsoNormal"><span style="color:rgb(31,73,125)">Andras</span><span style="color:rgb(136,136,136)"><u></u><u></u></span></p>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</blockquote>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
</div>
</div></div></div>
</div>
</blockquote></div><br></div></div></div></div></div>
</blockquote></div><br></div></div></div></div>
<br></div></div><span>______________________________<wbr>_________________<br>
Powered by <a href="http://www.kitware.com" rel="noreferrer" target="_blank">www.kitware.com</a><br>
<br>
Visit other Kitware open-source projects at <a href="http://www.kitware.com/opensource/opensource.html" rel="noreferrer" target="_blank">http://www.kitware.com/opensou<wbr>rce/opensource.html</a><br>
<br>
Search the list archives at: <a href="http://markmail.org/search/?q=vtk-developers" rel="noreferrer" target="_blank">http://markmail.org/search/?q=<wbr>vtk-developers</a><br>
<br>
Follow this link to subscribe/unsubscribe:<br>
<a href="http://public.kitware.com/mailman/listinfo/vtk-developers" rel="noreferrer" target="_blank">http://public.kitware.com/mail<wbr>man/listinfo/vtk-developers</a><br>
<br>
<br></span></blockquote></div><br></div></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div>