
<html><head></head><body>Hi,<br><br>I don't want an assertion for an error handler and I don't want an untested error handler when the code can trivially avoid needing one.<br><br>Which is exactly the current situation. With size_t, you can't overflow. If it's not broken, don't fix it.<br><br><div class="gmail_quote">Le 28 septembre 2020 01:51:37 GMT+03:00, Lyndon Brown <jnqnfe@gmail.com> a écrit :<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">

<pre class="k9mail">On Sun, 2020-09-27 at 23:38 +0300, Rémi Denis-Courmont wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">Le sunnuntaina 27. syyskuuta 2020, 23.03.24 EEST Lyndon Brown a écrit<br>:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #ad7fa8; padding-left: 1ex;">I don't think it's very fair to call the patch broken, an assert is<br>added to the function where options are added into the system as a<br>precaution to catching the unlikely scenario of there ever being<br>such a<br>bug causing a huge volume of creation requests.<br></blockquote>It is broken. It's trivial to construct a scenario where the option<br>list will <br>overflow. And as a general rule, we do not use assertions to handle<br>errors.<br></blockquote><br>I assume you mean like using a loop in the plugin descriptor? Sure, it<br>is of course trivially easy to do, but you're only going to end up with<br>a vast quantity of options if you're deliberately doing stupid things<br>to mess about, or if you make a mistake doing something very unusual.<br>Pragmatically it's not something we should expect to ever encounter in<br>the real world. Regardless, out of caution, this introduced a condition<br>to catch such a situation, which is arguably something worth adding<br>anyway.<br><br>I can't understand how you can consider that concern to be cause for<br>calling the patch 'broken', especially when the patch both handles the<br>situation (even if you don't like how), and arguably does so better.<br><br>If you'd prefer handling the error condition to be done in a different<br>way to asserting, then I would be perfectly happy to make such an<br>adjustment, you've only got to ask. I'd also be perfectly happy to<br>split out introduction of the check into it's own commit, since it's a<br>perfectly good enhancement in its own right. I think the main reason I<br>went with an assertion was simply because multiple existing parts<br>of vlc_plugin_desc_cb() make use of assertions to handle unexpected<br>problems, like too many shortcuts for instance. I actually do already<br>have another patch to submit later which replaces this assert with<br>error handling, and I can bring that bit forward if that appeases you.<br>(In fact as it happens, I've got later work to submit which<br>revises vlc_plugin_desc_cb() to avoid using assertions entirely such<br>that a problematic plugin gets rejected rather than bringing the whole<br>app down).<hr>vlc-devel mailing list<br>To unsubscribe or modify your subscription options:<br><a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a></pre></blockquote></div><br>-- <br>Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.</body></html>