https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=665
Summary: Empty constructor of JntArrayVel and JntArrayAcc
asserts
Product: KDL
Version: kdl-trunk
Platform: All
OS/Version: All
Status: NEW
Severity: blocker
Priority: P3
Component: Primitives
AssignedTo: orocos-dev [..] ...
ReportedBy: meeussen [..] ...
CC: orocos-dev [..] ...
Estimated Hours: 0.0
$ svn diff jntarrayvel.hpp jntarrayacc.hpp
Index: jntarrayvel.hpp
===================================================================
--- jntarrayvel.hpp (revision 30175)
+++ jntarrayvel.hpp (working copy)
When constructing a JntArrayVel or a JntArrayAcc without arguments, they
internally construct multiple JntArray's, with size = 0; However, the JntArray
has an assert(0 < size). The patch below adds a new empty constructor for a
JntArrayVel and a JntArrayAcc, which explicitly calls the empty constructor of
a JntArray.
@@ -35,7 +35,8 @@
JntArray q;
JntArray qdot;
public:
- JntArrayVel(unsigned int size=0);
+ JntArrayVel(){};
+ JntArrayVel(unsigned int size);
JntArrayVel(const JntArray& q,const JntArray& qdot);
JntArrayVel(const JntArray& q);
Index: jntarrayacc.hpp
===================================================================
--- jntarrayacc.hpp (revision 30175)
+++ jntarrayacc.hpp (working copy)
@@ -36,7 +36,8 @@
JntArray qdot;
JntArray qdotdot;
public:
- JntArrayAcc(unsigned int size=0);
+ JntArrayAcc(){};
+ JntArrayAcc(unsigned int size);
JntArrayAcc(const JntArray& q,const JntArray& qdot,const JntArray&
qdotdot);
JntArrayAcc(const JntArray& q,const JntArray& qdot);
JntArrayAcc(const JntArray& q);
[Bug 665] Empty constructor of JntArrayVel and JntArrayAcc asser
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=665
Ruben Smits <ruben [dot] smits [..] ...> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |ruben [dot] smits [..] ...en.b
| |e
Resolution| |FIXED
Status|NEW |RESOLVED
--- Comment #3 from Ruben Smits <ruben [dot] smits [..] ...> 2009-06-15 13:01:07 ---
svn ci -m "Close bug 665: Empty constructor of JntArrayVel and JntArrayAcc
asserts, thanks wim for the patch"
Sending src/jntarrayacc.hpp
Sending src/jntarrayvel.hpp
Transmitting file data ..
Committed revision 30211.
[Bug 665] Empty constructor of JntArrayVel and JntArrayAcc asser
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=665
--- Comment #2 from Wim Meeussen <meeussen [..] ...> 2009-06-09 18:16:45 ---
Created an attachment (id=442)
--> (https://www.fmtc.be/bugzilla/orocos/attachment.cgi?id=442)
As requested, the same patch as in the original message, as attachment
[Bug 665] Empty constructor of JntArrayVel and JntArrayAcc asser
https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=665
Peter Soetens <peter [..] ...> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |peter [..] ...
--- Comment #1 from Peter Soetens <peter [..] ...> 2009-06-08 10:33:10 ---
(In reply to comment #0)
> $ svn diff jntarrayvel.hpp jntarrayacc.hpp
> Index: jntarrayvel.hpp
> ===================================================================
> --- jntarrayvel.hpp (revision 30175)
> +++ jntarrayvel.hpp (working copy)
> When constructing a JntArrayVel or a JntArrayAcc without arguments, they
> internally construct multiple JntArray's, with size = 0; However, the JntArray
> has an assert(0 < size). The patch below adds a new empty constructor for a
> JntArrayVel and a JntArrayAcc, which explicitly calls the empty constructor of
> a JntArray.
Asserting on bad user input is *very* bad practice. First, it will not be
detected when the code is compiled with -DNDEBUG, second, you should only check
*internal* constraints with asserts, third, bad user input should cause an
exception to be thrown, especially in constructors, such that there is at least
a chance for recovery. No production code should ever contain asserts (ie be
compiled with -DNDEBUG).
Peter
[Bug 665] Empty constructor of JntArrayVel and JntArrayAcc asser
On Jun 8, 2009, at 04:33 , Peter Soetens wrote:
> https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=665
>
>
> Peter Soetens <peter [..] ...> changed:
>
> What |Removed |Added
> ----------------------------------------------------------------------------
> CC| |peter [..] ...
>
>
>
>
> --- Comment #1 from Peter Soetens <peter [..] ...>
> 2009-06-08 10:33:10 ---
> (In reply to comment #0)
>> $ svn diff jntarrayvel.hpp jntarrayacc.hpp
>> Index: jntarrayvel.hpp
>> ===================================================================
>> --- jntarrayvel.hpp (revision 30175)
>> +++ jntarrayvel.hpp (working copy)
>> When constructing a JntArrayVel or a JntArrayAcc without arguments,
>> they
>> internally construct multiple JntArray's, with size = 0; However,
>> the JntArray
>> has an assert(0 < size). The patch below adds a new empty
>> constructor for a
>> JntArrayVel and a JntArrayAcc, which explicitly calls the empty
>> constructor of
>> a JntArray.
>
> Asserting on bad user input is *very* bad practice. First, it will
> not be
> detected when the code is compiled with -DNDEBUG, second, you should
> only check
> *internal* constraints with asserts, third, bad user input should
> cause an
> exception to be thrown, especially in constructors, such that there
> is at least
> a chance for recovery. No production code should ever contain
> asserts (ie be
> compiled with -DNDEBUG).
There are different opinions on this. I know of plenty of production
code (including code on multi-million dollar satellites) deliberately
shipped with asserts in it. Now what action the assert takes when
fired is usually not the default, that I'll agree with, but if an
assertion can catch a bug before it propogates and causes more chaos,
or harm, or simply makes it harder to debug, I'd rather fire the
assert and try to do something intelligent.
Certainly, checking input directly from the user should not cause an
assert.
We've also had the discussion on this list before about throwing
exceptions and the implications of that (particularly in an embedded
environment). This isn't a black and white issue with one right answer.
YMMV
S
[Bug 665] Empty constructor of JntArrayVel and JntArrayAcc asser
On Mon, Jun 8, 2009 at 10:33 AM, Peter
Soetens<Peter [dot] Soetens [..] ...> wrote:
> https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=665
>
>
> Peter Soetens <peter [..] ...> changed:
>
> What |Removed |Added
> ----------------------------------------------------------------------------
> CC| |peter [..] ...
>
>
>
>
> --- Comment #1 from Peter Soetens <peter [..] ...> 2009-06-08 10:33:10 ---
> (In reply to comment #0)
>> $ svn diff jntarrayvel.hpp jntarrayacc.hpp
>> Index: jntarrayvel.hpp
>> ===================================================================
>> --- jntarrayvel.hpp (revision 30175)
>> +++ jntarrayvel.hpp (working copy)
>> When constructing a JntArrayVel or a JntArrayAcc without arguments, they
>> internally construct multiple JntArray's, with size = 0; However, the JntArray
>> has an assert(0 < size). The patch below adds a new empty constructor for a
>> JntArrayVel and a JntArrayAcc, which explicitly calls the empty constructor of
>> a JntArray.
>
> Asserting on bad user input is *very* bad practice.
Is there any list anywhere which lists all this bad practices for
those under us who are no computer scientists?
> First, it will not be
> detected when the code is compiled with -DNDEBUG,
That was intended, but apparently used incorrectly.
>second, you should only check
> *internal* constraints with asserts,
What does this mean, *internal* constraints? How/When can I use an
assert correctly?
>third, bad user input should cause an
> exception to be thrown, especially in constructors, such that there is at least
> a chance for recovery.
>No production code should ever contain asserts (ie be
> compiled with -DNDEBUG).
So, production code should is always compiled with -DNDEBUG, but AFAIK
the assertions only work when you compile without this flag, so how is
this different?
Ruben
> Peter
>
> --
> Configure bugmail: https://www.fmtc.be/bugzilla/orocos/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You are on the CC list for the bug.
> You are the assignee for the bug.
> --
> Orocos-Dev mailing list
> Orocos-Dev [..] ...
> http://lists.mech.kuleuven.be/mailman/listinfo/orocos-dev
>
[Bug 665] Empty constructor of JntArrayVel and JntArrayAcc asser
On Mon, Jun 8, 2009 at 11:00, Ruben Smits<ruben [dot] smits [..] ...> wrote:
> On Mon, Jun 8, 2009 at 10:33 AM, Peter
> Soetens<Peter [dot] Soetens [..] ...> wrote:
>> https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=665
>>
>>
>> Peter Soetens <peter [..] ...> changed:
>>
>> What |Removed |Added
>> ----------------------------------------------------------------------------
>> CC| |peter [..] ...
>>
>>
>>
>>
>> --- Comment #1 from Peter Soetens <peter [..] ...> 2009-06-08 10:33:10 ---
>> (In reply to comment #0)
>>> $ svn diff jntarrayvel.hpp jntarrayacc.hpp
>>> Index: jntarrayvel.hpp
>>> ===================================================================
>>> --- jntarrayvel.hpp (revision 30175)
>>> +++ jntarrayvel.hpp (working copy)
>>> When constructing a JntArrayVel or a JntArrayAcc without arguments, they
>>> internally construct multiple JntArray's, with size = 0; However, the JntArray
>>> has an assert(0 < size). The patch below adds a new empty constructor for a
>>> JntArrayVel and a JntArrayAcc, which explicitly calls the empty constructor of
>>> a JntArray.
>>
>> Asserting on bad user input is *very* bad practice.
>
> Is there any list anywhere which lists all this bad practices for
> those under us who are no computer scientists?
Fortunately, there is a very good book that should be on every C++
programmer's *desk*.
It's called: "C++ Coding Standards - 101 Rules, Guidelines and Best Practices"
from Herb Sutter and Andrei Alexandrescu. Here applies rule #68:
"Assert liberally to
document internal assumptions and invariants." , meaning: if you have
a public api
'foo(a,b)' you don't use asserts to check the ranges of 'a' and 'b',
since they are user input.
within foo(), you may use asserts to check for example, that after a
complex calculation,
the result is smaller than 'b' (if not, it's a but in foo's code.
It's one of the best and easy to understand C++ books out there.
>
>> First, it will not be
>> detected when the code is compiled with -DNDEBUG,
>
> That was intended, but apparently used incorrectly.
>
>>second, you should only check
>> *internal* constraints with asserts,
>
> What does this mean, *internal* constraints? How/When can I use an
> assert correctly?
see above.
>
>>third, bad user input should cause an
>> exception to be thrown, especially in constructors, such that there is at least
>> a chance for recovery.
>
>
>
>>No production code should ever contain asserts (ie be
>> compiled with -DNDEBUG).
>
> So, production code should is always compiled with -DNDEBUG, but AFAIK
> the assertions only work when you compile without this flag, so how is
> this different?
Well, assertions are for *you*, the developer, never for the user,
unless that user
is eager to debug *your* code.
Peter