[quagga-dev 12881] Re: [PATCH 2/7] Fixed logically dead code in lib/stream.c

Donald Sharp sharpd at cumulusnetworks.com
Sat Jul 25 16:10:55 BST 2015


Greg -

These scan id's are from coverity.  They provide a open source scanning
tool.  I assumed since one of the maintainers was running the scan that it
was a well known resource for Quagga:

https://scan5.coverity.com:8443/reports.htm#v40850/p10064

Hence my request for Denil to put the scan id in, it seemed more than
reasonable at the time.

As for the assert, since I knew that asserts were always compiled into the
code, I figured the removal of the if statement was the correct thing to
do, hence my instructions to Denil to do so.

donald

On Sat, Jul 25, 2015 at 9:43 AM, Greg Troxel <gdt at ir.bbn.com> wrote:

>
> nolan <nolan at cumulusnetworks.com> writes:
> (with top-posting repaired!)
>
> > On 07/23/2015 06:52 AM, Denil Vira wrote:
> >> Coverity Scan ID 1302488. Test for size==0 makes no sense, since assert
> immediately before it
> >> would not let this code happen.
>
> What namespace are these scan ids in?  Are they someplace public that's
> committed to existing for a lont time?  If they aren't, I would prefer
> that they not be in the commit messages.
>
> > Asserts are sometimes compiled out, so the redundant check is
> > necessary. The optimizer will cull it if asserts are enabled, so it
> > doesn't hurt the generated code.
>
> Really we need rules in our style/design guide that addresses this, and
> then we can evaluate the code against the rules.  It's an entirely
> reasonable style (known as design by contract) to have asserts at the
> beginning of a function to check the documented calling conventions.
> Then, one would expect the asserts to remain in production.   Having
> asserts and then conditionals to return errors seems overly complex to me.
>
> _______________________________________________
> Quagga-dev mailing list
> Quagga-dev at lists.quagga.net
> https://lists.quagga.net/mailman/listinfo/quagga-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.quagga.net/pipermail/quagga-dev/attachments/20150725/1c2fb1cf/attachment-0001.html>


More information about the Quagga-dev mailing list