to throw or not to throw

classic Classic list List threaded Threaded
44 messages Options
123
Reply | Threaded
Open this post in threaded view
|

to throw or not to throw

Radu B. Rusu
Administrator
Simple Q, with a very complicated A:

  * when do we throw exceptions and when do we return error codes via return types or member variables?

Also, when to output things via PCL_XXX macros?

There's many pros and cons, and you can see the world's C++ gurus being split into multiple (yes, more than 2) camps on
this one. It's time we decide as well, and take one side, and just enforce consistency throughout our code base.


The way I see it, we should throw exceptions when we really want to catch our users' attention, as returning FALSE on
some method will most likely be ignored. So:

  * cat. CRITICAL/IMPORTANT/FAILURE: throw exception when the result is unexpected
  * cat. ERROR/UNSUCCESSFUL/STATUS: return type + PCL_XXX output

We also need to work to make PCL_XXX redirect to user given ostreams. I like this pattern a lot:

https://github.com/gbiggs/hokuyoaist/blob/master/include/hokuyoaist/sensor.h, line 134.

If a user wants output to the console, they pass cout/cerr/clog to the constructor. Clean, efficient, simple.

Cheers,
Radu.
--
Point Cloud Library (PCL) - http://pointclouds.org
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Jochen Sprickerhof
Administrator
* Radu Rusu <[hidden email]> [2011-09-21 19:55]:

> Simple Q, with a very complicated A:
>
>  * when do we throw exceptions and when do we return error codes via return types or member variables?
>
> Also, when to output things via PCL_XXX macros?
>
> There's many pros and cons, and you can see the world's C++ gurus
> being split into multiple (yes, more than 2) camps on this one. It's
> time we decide as well, and take one side, and just enforce
> consistency throughout our code base.
>
>
> The way I see it, we should throw exceptions when we really want to
> catch our users' attention, as returning FALSE on some method will
> most likely be ignored. So:
>
>  * cat. CRITICAL/IMPORTANT/FAILURE: throw exception when the result is unexpected
>  * cat. ERROR/UNSUCCESSFUL/STATUS: return type + PCL_XXX output

I'm not sure about these categories, basically we have two users (i.e.
application developers and end-users) and problems caused by both, as
well. For problems caused by developers throwing an exception sounds
right [1]. For problems caused by users [2] we should return a hint.

> We also need to work to make PCL_XXX redirect to user given ostreams. I like this pattern a lot:
>
> https://github.com/gbiggs/hokuyoaist/blob/master/include/hokuyoaist/sensor.h, line 134.
>
> If a user wants output to the console, they pass cout/cerr/clog to the constructor. Clean, efficient, simple.

Veto. We should not make assumptions on what application developers use.
Maybe they want to localize the message or feed it into a GUI and that
would be really hard if it's a stream already.  Basically I would like
to get rid of all PCL_XXX and cout stuff because it's carrying
information without providing easy access to it.  I haven't investigated
the alternatives, but one way would be to provide custom exceptions that
encapsulate the message.

> Cheers,
> Radu.

[1] For example the problem with pcl::copyPointClouds () I looked into
today. Here we want to warn the developer that calling it with
input == output is not supported. So throwing an exception should be the
right solution.

[2] Think of a nice ICP GUI where the users can set the parameters and
the "relax parameters" message.

Cheers,

Jochen
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Mourad

> We also need to work to make PCL_XXX redirect to user given ostreams. I like this pattern a lot:
>
> https://github.com/gbiggs/hokuyoaist/blob/master/include/hokuyoaist/sensor.h, line 134.
>
> If a user wants output to the console, they pass cout/cerr/clog to the constructor. Clean, efficient, simple.

Veto. We should not make assumptions on what application developers use.
Maybe they want to localize the message or feed it into a GUI and that
would be really hard if it's a stream already.  Basically I would like
to get rid of all PCL_XXX and cout stuff because it's carrying
information without providing easy access to it.  I haven't investigated
the alternatives, but one way would be to provide custom exceptions that
encapsulate the message.


+1 for having a more flexible logging functionality in PCL. 

I have a solution draft based on C++ streams, see a minimal working example in the attached file. 

The idea is to override the sync() method of a std::stringbuf object and redirect the content of the buffer.

The user can easily define his own logger (to feed a GUI or anything else) by sub-classing from LoggerBuffer (which inherits from std::stringbuf).

You'll find two examples of implementations : DefaultLoggerBuffer which logs to the console, and FileLoggerBuffer which logs to a file.

Cheers,
Mourad


_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org

logger.cpp (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Radu B. Rusu
Administrator
Mourad, Jochen,

While these solutions are great, I don't think a library needs to do logging. That falls onto the application
developers. Anything like log4cxx will be vetoed :) too.

PCL_XXX is good and we shouldn't get rid of it. It's a macro, and we can make it do whatever we want: throw exceptions
if you want. There shouldn't be any cout/cerr anywhere in the codebase. If you see it, fix it without asking please.

Now, Mourad's suggestion is not far from what I mentioned earlier (Geoff's way), and I'm sure we can find a way to
incorporate this in.

Jochen, I would avoid us throwing exceptions in every function, when input == output for example. Remember that throwing
exceptions can cause additional problems... there might be still compilers that do not inline functions because of that
+ it makes the user's code very ugly. We're not Java programmers :) That being said, this is exactly the reason why I
opened this thread (call it "cup" of worms) because it would be nice to be consistent in every of the libraries that we
write and stamp as "PCL", independent of what the decision is.

Cheers,
Radu.
--
Point Cloud Library (PCL) - http://pointclouds.org

On 09/22/2011 05:53 AM, Mourad Boufarguine wrote:

>
>      > We also need to work to make PCL_XXX redirect to user given ostreams. I like this pattern a lot:
>      >
>      > https://github.com/gbiggs/hokuyoaist/blob/master/include/hokuyoaist/sensor.h, line 134.
>      >
>      > If a user wants output to the console, they pass cout/cerr/clog to the constructor. Clean, efficient, simple.
>
>     Veto. We should not make assumptions on what application developers use.
>     Maybe they want to localize the message or feed it into a GUI and that
>     would be really hard if it's a stream already.  Basically I would like
>     to get rid of all PCL_XXX and cout stuff because it's carrying
>     information without providing easy access to it.  I haven't investigated
>     the alternatives, but one way would be to provide custom exceptions that
>     encapsulate the message.
>
>
> +1 for having a more flexible logging functionality in PCL.
>
> I have a solution draft based on C++ streams, see a minimal working example in the attached file.
>
> The idea is to override the sync() method of a std::stringbuf object and redirect the content of the buffer.
>
> The user can easily define his own logger (to feed a GUI or anything else) by sub-classing from LoggerBuffer (which
> inherits from std::stringbuf).
>
> You'll find two examples of implementations : DefaultLoggerBuffer which logs to the console, and FileLoggerBuffer which
> logs to a file.
>
> Cheers,
> Mourad
>
>
>
> _______________________________________________
> PCL-developers mailing list
> [hidden email]
> http://pointclouds.org/mailman/listinfo/pcl-developers
> http://pointclouds.org
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Michael Dixon-2
I vote for using exceptions as our predominant error handling model.
There are arguments for both sides, but here are the reasons I think
exceptions are preferable:

1) Exceptions add a new channel of information, which let's you
preserve a function's return value for more relevant data.  That is,
if you use return codes, you can't use a function's return values for
actual data/results.  So you have to abandon syntax like this:
    PointCloud cloud = loadPointCloud ("file.pcd");
in favor of:
    PointCloud cloud;
    int return_code = loadPointCloud ("file.pcd", cloud);
Everyone is familiar with both styles, but I personally think that the
first way is more readable, and I think it's always more intuitive
when functions actually return their result.

2) Exceptions can store lots of important information that can be used
when handling an error, so the information needed to interpret the
error and decide how to handle it is typically self-contained.  The
exception can represent exactly why it was thrown and store an
informative error message in case you want to display it or log it.

3) When you make people explicitly check return codes, they're forced
to clutter up their code with a bunch of if-else's, which I think
really hurts the readability.  Perhaps worse, people often just
neglect to do any checking.  That's often fine... until they actually
encounter an error.  Then instead of getting an obvious uncaught
exception that explains to them exactly what went wrong and where,
they just get weird behavior, and we get questions on pcl-users@.

Also, if you don't mind a few more of my cents (I've already given you
way more than two), I wanted to respond to a couple things you
mentioned in your email.

> Jochen, I would avoid us throwing exceptions in every function, when input
> == output for example.

This sort of in-place operation probably won't cause an error in 2.0,
but for the sake of argument, I'd say this is still a situation where
you want an exception thrown.  When correct behavior is to halt the
flow of execution, and when the error may need to propagate out of
several layers of functions, why not use an exception?  This is pretty
much what they're designed for.  This particular example isn't an
error you can safely ignore, and return codes are very easy to ignore.

> Remember that throwing exceptions can cause
> additional problems... there might be still compilers that do not inline
> functions because of that + it makes the user's code very ugly.

I'm not familiar with the issue of exceptions preventing inlining.  Do
you have any good resources I could read on this?  I tried to google
it, and I didn't find any good explanation of it.  This discussion
seems to indicate that throwing exceptions doesn't prevent inlining:
    http://stackoverflow.com/questions/7494399/would-c-exception-stops-function-from-being-inlined
and this one:
    http://www.gotw.ca/publications/mill22.htm
mentions that "some compilers will automatically refuse to inline a
function having an exception *specification*". But the solution to
that problem is just to not use exception specifications (from the
same article: "Moral #1: Never write an exception specification").
This advice is echoed in the following discussion:
    http://www.cplusplus.com/forum/beginner/13091/
As far as I can tell, there's not really a good reason to specify
which exceptions a function might throw in its declaration.  But
that's not an argument against using exceptions.  If you need to throw
an exception, just throw it.

As for exceptions making code ugly?  I'd love for you to elaborate
with an example, because I'm not sure I understand what you're getting
at.  Personally, I think it's exactly the opposite.  Without
exceptions, your code has to look like this:

    PointCloud cloud;
    if (pcl::loadPointCloud ("file.pcd", cloud) == ERROR)
      return ERROR;

    PointCloud filtered;
    if (pcl::doSomeFiltering (cloud, filtered) == ERROR)
      return ERROR;

    PointCloud features;
    if (pcl::computeSomeFeatures (filtered, features) == ERROR)
      return ERROR;

This is pretty cluttered.  Plus, I really don't like putting
statements (code that "does something") inside of "if" expressions,
because I think it sort of hides them.  So I prefer to break it out
even more, like this:

    int return_code;

    PointCloud cloud;
    return_code = pcl::loadPointCloud ("file.pcd", cloud);
    if (return_code == ERROR)
      return ERROR;

    PointCloud filtered;
    return_code = pcl::doSomeFiltering (cloud, filtered);
    if (return_code == ERROR)
      return ERROR;

    PointCloud features;
    return_code = pcl::computeSomeFeatures (filtered, features);
    if (return_code == ERROR)
      return ERROR;

but then your code just gets even more verbose.  On the other hand, if
you use exceptions, the same operations can be expressed as:

    PointCloud cloud = pcl::loadPointCloud ("file.pcd");
    PointCloud filtered = pcl::doSomeFiltering (cloud);
    PointCloud features = pcl::computeSomeFeatures (filtered);

The error handling still happens, but it's all implicit.  Unless you
have custom error handling logic that's particular to your specific
application, then I don't think you ought to be required to write any
error handling code at all.  By default, when something goes wrong,
the exception will halt execution and propagate its way out, which is
what you'd have needed to write manually with return codes.

> We're not Java programmers :)

Thank goodness!  But I still like exceptions!

> That being said, this is exactly the reason why I opened
> this thread (call it "cup" of worms) because it would be nice to be
> consistent in every of the libraries that we write and stamp as "PCL",
> independent of what the decision is.

I totally agree.  Thanks for kicking this discussion off!  It's
already motivated me to read up a bit more on this issue, and I'm
looking forward to learning more from the rest of the PCL devs.

-Michael

> Cheers,
> Radu.
> --
> Point Cloud Library (PCL) - http://pointclouds.org
>
> On 09/22/2011 05:53 AM, Mourad Boufarguine wrote:
>>
>>     > We also need to work to make PCL_XXX redirect to user given
>> ostreams. I like this pattern a lot:
>>     >
>>     >
>> https://github.com/gbiggs/hokuyoaist/blob/master/include/hokuyoaist/sensor.h,
>> line 134.
>>     >
>>     > If a user wants output to the console, they pass cout/cerr/clog to
>> the constructor. Clean, efficient, simple.
>>
>>    Veto. We should not make assumptions on what application developers
>> use.
>>    Maybe they want to localize the message or feed it into a GUI and that
>>    would be really hard if it's a stream already.  Basically I would like
>>    to get rid of all PCL_XXX and cout stuff because it's carrying
>>    information without providing easy access to it.  I haven't
>> investigated
>>    the alternatives, but one way would be to provide custom exceptions
>> that
>>    encapsulate the message.
>>
>>
>> +1 for having a more flexible logging functionality in PCL.
>>
>> I have a solution draft based on C++ streams, see a minimal working
>> example in the attached file.
>>
>> The idea is to override the sync() method of a std::stringbuf object and
>> redirect the content of the buffer.
>>
>> The user can easily define his own logger (to feed a GUI or anything else)
>> by sub-classing from LoggerBuffer (which
>> inherits from std::stringbuf).
>>
>> You'll find two examples of implementations : DefaultLoggerBuffer which
>> logs to the console, and FileLoggerBuffer which
>> logs to a file.
>>
>> Cheers,
>> Mourad
>>
>>
>>
>> _______________________________________________
>> PCL-developers mailing list
>> [hidden email]
>> http://pointclouds.org/mailman/listinfo/pcl-developers
>> http://pointclouds.org
>
> _______________________________________________
> PCL-developers mailing list
> [hidden email]
> http://pointclouds.org/mailman/listinfo/pcl-developers
> http://pointclouds.org
>
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Radu B. Rusu
Administrator
I knew we will get into polemics, that's a given with e-mails like this :) Next on the list: "vim vs emacs" :)


So there's rules of thumb about "not abusing" exceptions, which could come in handy. I usually inspire myself from:

  * http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Exceptions

  * Scott Meyers's wisdom that throwing an exception is about "three orders of magnitude slower" than a normal return.

  * POLA (Principle Of Least Astonishment) violations when it's simply not obvious/straightforward what a method will
do. If we use exceptions everywhere, this can happen.

  * Dave Abraham's http://www.boost.org/community/error_handling.html:

"A more appropriate question to ask is: ``do we want stack unwinding here?'' Because actually handling an exception is
likely to be significantly slower than executing mainline code, you should also ask: ``Can I afford stack unwinding
here?'' For example, a desktop application performing a long computation might periodically check to see whether the
user had pressed a cancel button. Throwing an exception could allow the operation to be cancelled gracefully. On the
other hand, it would probably be inappropriate to throw and handle exceptions in the inner loop of this computation
because that could have a significant performance impact. The guideline mentioned above has a grain of truth in it: in
time critical code, throwing an exception should be the exception, not the rule."

Personally, I like Boost's way of doing things a lot (or I just got used to it). The example that I can think of is
polymorphic conversions via dynamic_cast:
- there are cases where dynamic_cast produces an exception (std::bad_cast) if the conversion is not possible when used
on a reference type. Because null references do not exist in C++, the conversion MUST succeed and result in a valid
reference, or it fails and you get an exception
- there are other cases where you want to convert pointer types and there it simply returns a null pointer in case of an
error

  * inlining is a problem indeed. Try the following simple example:

   1 #include <stdexcept>
   2 #include <iostream>
   3
   4 inline void
   5 foo ()
   6 {
   7   int a = 2;
   8   if (a == 2)
   9     throw new std::runtime_error ("test");
  10 }
  11
  12 inline void
  13 bar ()
  14 {
  15   int b = 4;
  16   if (b == 4)
  17     std::cerr << "test" << std::endl;
  18 }
  19
  20
  21 int main (int argc, char **argv)
  22 {
  23   bar ();
  24   foo ();
  25   return (0);
  26 }

Compile with -s (-O1-3 not -O0 because that disables inlining completely) and analyze the assembly output. You'll see an
extra call in there for foo, while the code for bar inlined nicely. Changing the type of the exception to anything else
doesn't seem to help -- just can't get rid of the extra "call _Z3foov". So even without an exception specification, the
compiler is unhappy. And this is GCC -- we would like to support as many compilers as possible, and inducing performance
penalties like this stinks, and people will hate us.

[Edit: I tried the same with pathCC and ICC. Same result :(]

Personally I think there is a good reason for this. Throwing an exception invokes adding a new path of control flow to
the program, which means it's impossible for the compiler not to issue a long jump. This involves things like pushing
registers, jumping, returning and popping them, which is what breaks performance.

Of course there are probably "exceptions" to this too :)


  * another pitfall that is advertised almost everywhere is how to deal within your code with functions that throw
exceptions that call functions that throw exceptions (phew), etc. You definitely do not want:

try
{
...
}
catch (...)
{
...
}

  * the reason why I mentioned we're not Java programmers, is that exceptions in Java are actually more powerful as they
are checked and enforced at compile-time. Big difference.




This being said, we're not part of "the commonwealth". We're "unique snow flowers", and adepts of "one hat size doesn't
fit all". :) So forgetting about what I said above (which is mostly cited from what others have said), let's continue
and see what we can find that's best for us.


Michael has a great point that we omitted discussing yet. In PCL 2.0 we would like to use return types more, and be able
to easily chain and concatenate operators, pretty much like Eigen does (without the fancy lazy evaluation :D).



On 09/22/2011 10:07 AM, Michael Dixon wrote:

> I vote for using exceptions as our predominant error handling model.
> There are arguments for both sides, but here are the reasons I think
> exceptions are preferable:
>
> 1) Exceptions add a new channel of information, which let's you
> preserve a function's return value for more relevant data.  That is,
> if you use return codes, you can't use a function's return values for
> actual data/results.  So you have to abandon syntax like this:
>      PointCloud cloud = loadPointCloud ("file.pcd");
> in favor of:
>      PointCloud cloud;
>      int return_code = loadPointCloud ("file.pcd", cloud);

This means that:

 >      PointCloud cloud = loadPointCloud ("file.pcd");

+1

 >      PointCloud cloud;
 >      int return_code = loadPointCloud ("file.pcd", cloud);

-1

Totally agreed.

> 2) Exceptions can store lots of important information that can be used
> when handling an error, so the information needed to interpret the
> error and decide how to handle it is typically self-contained.  The
> exception can represent exactly why it was thrown and store an
> informative error message in case you want to display it or log it.

Agreed. This votes for using something "like exceptions" as well.

> 3) When you make people explicitly check return codes, they're forced
> to clutter up their code with a bunch of if-else's, which I think
> really hurts the readability.  Perhaps worse, people often just
> neglect to do any checking.  That's often fine... until they actually
> encounter an error.  Then instead of getting an obvious uncaught
> exception that explains to them exactly what went wrong and where,
> they just get weird behavior, and we get questions on pcl-users@.

So this is an important distinction. If we agree that the output of a filter which returns 0 points back (and thus
filters everything) is not a CRITICAL ERROR but rather an EXPECTED result from that operation, then the user has the
responsibility to check for it, and not us. If however the parameters would be chosen in such a way that the filter
would crash and produce -1 points (hehe :) ), then we would need to throw an exception, because it is UNEXPECTED for the
user to receive -1 points. :) Well, silly fictitious example, but I hope it makes a point.


> I'm not familiar with the issue of exceptions preventing inlining.  Do
> you have any good resources I could read on this?  I tried to google
> it, and I didn't find any good explanation of it.  This discussion
> seems to indicate that throwing exceptions doesn't prevent inlining:
>      http://stackoverflow.com/questions/7494399/would-c-exception-stops-function-from-being-inlined
> and this one:
>      http://www.gotw.ca/publications/mill22.htm
> mentions that "some compilers will automatically refuse to inline a
> function having an exception *specification*". But the solution to
> that problem is just to not use exception specifications (from the
> same article: "Moral #1: Never write an exception specification").

See above. Also, stackoverflow has a lot of crappy answers and solutions too, just between us. :) I also did a search
and found silly things. The problem is that they are never checked by experts, so people just vote by themselves (like
Wikipedia).

> As for exceptions making code ugly?  I'd love for you to elaborate
> with an example, because I'm not sure I understand what you're getting
> at.  Personally, I think it's exactly the opposite.  Without
> exceptions, your code has to look like this:
[...]

> This is pretty cluttered.  Plus, I really don't like putting
> statements (code that "does something") inside of "if" expressions,
> because I think it sort of hides them.  So I prefer to break it out
> even more, like this:
>
>      int return_code;
>
>      PointCloud cloud;
>      return_code = pcl::loadPointCloud ("file.pcd", cloud);
>      if (return_code == ERROR)
>        return ERROR;
>
>      PointCloud filtered;
>      return_code = pcl::doSomeFiltering (cloud, filtered);
>      if (return_code == ERROR)
>        return ERROR;
>
>      PointCloud features;
>      return_code = pcl::computeSomeFeatures (filtered, features);
>      if (return_code == ERROR)
>        return ERROR;
>
> but then your code just gets even more verbose.  On the other hand, if
> you use exceptions, the same operations can be expressed as:
>
>      PointCloud cloud = pcl::loadPointCloud ("file.pcd");
>      PointCloud filtered = pcl::doSomeFiltering (cloud);
>      PointCloud features = pcl::computeSomeFeatures (filtered);

Hehe, this is not the same thing. This is: =)

try
{
   PointCloud cloud = pcl::loadPointCloud ("file.pcd");
}
catch (ExceptionFoo)
{
...
}
try
{
   PointCloud filtered = pcl::doSomeFiltering (cloud);
}
catch (ExceptionBar)
{
...
}
try
{
   PointCloud features = pcl::computeSomeFeatures (filtered);
}
catch (ExceptionEps)
{
...
}

Or:

try
{
   PointCloud cloud = pcl::loadPointCloud ("file.pcd");
   PointCloud filtered = pcl::doSomeFiltering (cloud);
   PointCloud features = pcl::computeSomeFeatures (filtered);
}
catch (ExceptionFoo)
{
...
}
catch (ExceptionBar)
{
...
}
catch (ExceptionEps)
{
...
}
catch (...) /// ????? (do you even have a way of knowing them all?)



What do others think? I personally like building a table with requirements. Michael kick started 2.0 into this
discussion which is really good. So:

  * we want to use return types for data and not for error codes

  * we want the code to be less cluttered and simpler

  * we want to avoid writing things to the console, yet provide meaningful messages/codes to the developer

  * we want HIGH performance no matter what

One idea would be to discourage throwing exceptions in functions that we assume can be inlined due to these performance
issues, but maybe propagate it more as Michael said throughout the code base, IF we agree that it simplifies our lives.
Read more below...


Btw, let's not forget that we're talking about the set of libraries in PCL and not the set of applications, higher level
tools, etc. So question: what would other libraries do? :) STL? Boost? etc.

We can also learn from Eigen. Eigen doesn't throw any exceptions. They use ASSERTS. They swear that "exceptions are
definitely a costly feature for performance, whereas asserts are cheap". To quote even further "exceptions partly break
the stack-based nature of the language, so they prevent the compiler from making optimizations".


PS. Awesome topic! I was kidding about "vim" vs "emacs" though so please keep it friendly ;)


Cheers,
Radu.
--
Point Cloud Library (PCL) - http://pointclouds.org
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Michael Dixon-2
Thanks for the detailed reply!  I've got lots to think about now.  One
quick response to the last point about "asserts":

I think asserts might work in just about every case where I was
imagining using exceptions.  They retain the benefits of not hijacking
the return values, and they also don't force users to scatter error
checking throughout all of their code.  The only problem is that they
don't let you recover from an error.  Of course, lots of our errors
are "unrecoverable" anyway.  If someone tries to pass bad inputs,
terminating with a failed assertion makes perfect sense to me.  (Of
course, we'll be screwing over anybody who may need to do some cleanup
operations, like finishing writing out to a file, before exiting. But
maybe that's okay.)

The remaining question then is if we have "exceptional" cases where
we'd like people to be able to catch them and continue running.  Those
situations require either exceptions or return codes (or some
as-of-yet-un-thought-of clever third alternative).  Can anybody think
of some examples of when those situations (i.e. recoverable
exceptions) could arise in PCL?

-Michael


On Thu, Sep 22, 2011 at 11:53 AM, Radu B. Rusu <[hidden email]> wrote:

> I knew we will get into polemics, that's a given with e-mails like this :)
> Next on the list: "vim vs emacs" :)
>
>
> So there's rules of thumb about "not abusing" exceptions, which could come
> in handy. I usually inspire myself from:
>
>  * http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Exceptions
>
>  * Scott Meyers's wisdom that throwing an exception is about "three orders
> of magnitude slower" than a normal return.
>
>  * POLA (Principle Of Least Astonishment) violations when it's simply not
> obvious/straightforward what a method will do. If we use exceptions
> everywhere, this can happen.
>
>  * Dave Abraham's http://www.boost.org/community/error_handling.html:
>
> "A more appropriate question to ask is: ``do we want stack unwinding here?''
> Because actually handling an exception is likely to be significantly slower
> than executing mainline code, you should also ask: ``Can I afford stack
> unwinding here?'' For example, a desktop application performing a long
> computation might periodically check to see whether the user had pressed a
> cancel button. Throwing an exception could allow the operation to be
> cancelled gracefully. On the other hand, it would probably be inappropriate
> to throw and handle exceptions in the inner loop of this computation because
> that could have a significant performance impact. The guideline mentioned
> above has a grain of truth in it: in time critical code, throwing an
> exception should be the exception, not the rule."
>
> Personally, I like Boost's way of doing things a lot (or I just got used to
> it). The example that I can think of is polymorphic conversions via
> dynamic_cast:
> - there are cases where dynamic_cast produces an exception (std::bad_cast)
> if the conversion is not possible when used on a reference type. Because
> null references do not exist in C++, the conversion MUST succeed and result
> in a valid reference, or it fails and you get an exception
> - there are other cases where you want to convert pointer types and there it
> simply returns a null pointer in case of an error
>
>  * inlining is a problem indeed. Try the following simple example:
>
>  1 #include <stdexcept>
>  2 #include <iostream>
>  3
>  4 inline void
>  5 foo ()
>  6 {
>  7   int a = 2;
>  8   if (a == 2)
>  9     throw new std::runtime_error ("test");
>  10 }
>  11
>  12 inline void
>  13 bar ()
>  14 {
>  15   int b = 4;
>  16   if (b == 4)
>  17     std::cerr << "test" << std::endl;
>  18 }
>  19
>  20
>  21 int main (int argc, char **argv)
>  22 {
>  23   bar ();
>  24   foo ();
>  25   return (0);
>  26 }
>
> Compile with -s (-O1-3 not -O0 because that disables inlining completely)
> and analyze the assembly output. You'll see an extra call in there for foo,
> while the code for bar inlined nicely. Changing the type of the exception to
> anything else doesn't seem to help -- just can't get rid of the extra "call
> _Z3foov". So even without an exception specification, the compiler is
> unhappy. And this is GCC -- we would like to support as many compilers as
> possible, and inducing performance penalties like this stinks, and people
> will hate us.
>
> [Edit: I tried the same with pathCC and ICC. Same result :(]
>
> Personally I think there is a good reason for this. Throwing an exception
> invokes adding a new path of control flow to the program, which means it's
> impossible for the compiler not to issue a long jump. This involves things
> like pushing registers, jumping, returning and popping them, which is what
> breaks performance.
>
> Of course there are probably "exceptions" to this too :)
>
>
>  * another pitfall that is advertised almost everywhere is how to deal
> within your code with functions that throw exceptions that call functions
> that throw exceptions (phew), etc. You definitely do not want:
>
> try
> {
> ...
> }
> catch (...)
> {
> ...
> }
>
>  * the reason why I mentioned we're not Java programmers, is that exceptions
> in Java are actually more powerful as they are checked and enforced at
> compile-time. Big difference.
>
>
>
>
> This being said, we're not part of "the commonwealth". We're "unique snow
> flowers", and adepts of "one hat size doesn't fit all". :) So forgetting
> about what I said above (which is mostly cited from what others have said),
> let's continue and see what we can find that's best for us.
>
>
> Michael has a great point that we omitted discussing yet. In PCL 2.0 we
> would like to use return types more, and be able to easily chain and
> concatenate operators, pretty much like Eigen does (without the fancy lazy
> evaluation :D).
>
>
>
> On 09/22/2011 10:07 AM, Michael Dixon wrote:
>>
>> I vote for using exceptions as our predominant error handling model.
>> There are arguments for both sides, but here are the reasons I think
>> exceptions are preferable:
>>
>> 1) Exceptions add a new channel of information, which let's you
>> preserve a function's return value for more relevant data.  That is,
>> if you use return codes, you can't use a function's return values for
>> actual data/results.  So you have to abandon syntax like this:
>>     PointCloud cloud = loadPointCloud ("file.pcd");
>> in favor of:
>>     PointCloud cloud;
>>     int return_code = loadPointCloud ("file.pcd", cloud);
>
> This means that:
>
>>      PointCloud cloud = loadPointCloud ("file.pcd");
>
> +1
>
>>      PointCloud cloud;
>>      int return_code = loadPointCloud ("file.pcd", cloud);
>
> -1
>
> Totally agreed.
>
>> 2) Exceptions can store lots of important information that can be used
>> when handling an error, so the information needed to interpret the
>> error and decide how to handle it is typically self-contained.  The
>> exception can represent exactly why it was thrown and store an
>> informative error message in case you want to display it or log it.
>
> Agreed. This votes for using something "like exceptions" as well.
>
>> 3) When you make people explicitly check return codes, they're forced
>> to clutter up their code with a bunch of if-else's, which I think
>> really hurts the readability.  Perhaps worse, people often just
>> neglect to do any checking.  That's often fine... until they actually
>> encounter an error.  Then instead of getting an obvious uncaught
>> exception that explains to them exactly what went wrong and where,
>> they just get weird behavior, and we get questions on pcl-users@.
>
> So this is an important distinction. If we agree that the output of a filter
> which returns 0 points back (and thus filters everything) is not a CRITICAL
> ERROR but rather an EXPECTED result from that operation, then the user has
> the responsibility to check for it, and not us. If however the parameters
> would be chosen in such a way that the filter would crash and produce -1
> points (hehe :) ), then we would need to throw an exception, because it is
> UNEXPECTED for the user to receive -1 points. :) Well, silly fictitious
> example, but I hope it makes a point.
>
>
>> I'm not familiar with the issue of exceptions preventing inlining.  Do
>> you have any good resources I could read on this?  I tried to google
>> it, and I didn't find any good explanation of it.  This discussion
>> seems to indicate that throwing exceptions doesn't prevent inlining:
>>
>> http://stackoverflow.com/questions/7494399/would-c-exception-stops-function-from-being-inlined
>> and this one:
>>     http://www.gotw.ca/publications/mill22.htm
>> mentions that "some compilers will automatically refuse to inline a
>> function having an exception *specification*". But the solution to
>> that problem is just to not use exception specifications (from the
>> same article: "Moral #1: Never write an exception specification").
>
> See above. Also, stackoverflow has a lot of crappy answers and solutions
> too, just between us. :) I also did a search and found silly things. The
> problem is that they are never checked by experts, so people just vote by
> themselves (like Wikipedia).
>
>> As for exceptions making code ugly?  I'd love for you to elaborate
>> with an example, because I'm not sure I understand what you're getting
>> at.  Personally, I think it's exactly the opposite.  Without
>> exceptions, your code has to look like this:
>
> [...]
>>
>> This is pretty cluttered.  Plus, I really don't like putting
>> statements (code that "does something") inside of "if" expressions,
>> because I think it sort of hides them.  So I prefer to break it out
>> even more, like this:
>>
>>     int return_code;
>>
>>     PointCloud cloud;
>>     return_code = pcl::loadPointCloud ("file.pcd", cloud);
>>     if (return_code == ERROR)
>>       return ERROR;
>>
>>     PointCloud filtered;
>>     return_code = pcl::doSomeFiltering (cloud, filtered);
>>     if (return_code == ERROR)
>>       return ERROR;
>>
>>     PointCloud features;
>>     return_code = pcl::computeSomeFeatures (filtered, features);
>>     if (return_code == ERROR)
>>       return ERROR;
>>
>> but then your code just gets even more verbose.  On the other hand, if
>> you use exceptions, the same operations can be expressed as:
>>
>>     PointCloud cloud = pcl::loadPointCloud ("file.pcd");
>>     PointCloud filtered = pcl::doSomeFiltering (cloud);
>>     PointCloud features = pcl::computeSomeFeatures (filtered);
>
> Hehe, this is not the same thing. This is: =)
>
> try
> {
>  PointCloud cloud = pcl::loadPointCloud ("file.pcd");
> }
> catch (ExceptionFoo)
> {
> ...
> }
> try
> {
>  PointCloud filtered = pcl::doSomeFiltering (cloud);
> }
> catch (ExceptionBar)
> {
> ...
> }
> try
> {
>  PointCloud features = pcl::computeSomeFeatures (filtered);
> }
> catch (ExceptionEps)
> {
> ...
> }
>
> Or:
>
> try
> {
>  PointCloud cloud = pcl::loadPointCloud ("file.pcd");
>  PointCloud filtered = pcl::doSomeFiltering (cloud);
>  PointCloud features = pcl::computeSomeFeatures (filtered);
> }
> catch (ExceptionFoo)
> {
> ...
> }
> catch (ExceptionBar)
> {
> ...
> }
> catch (ExceptionEps)
> {
> ...
> }
> catch (...) /// ????? (do you even have a way of knowing them all?)
>
>
>
> What do others think? I personally like building a table with requirements.
> Michael kick started 2.0 into this discussion which is really good. So:
>
>  * we want to use return types for data and not for error codes
>
>  * we want the code to be less cluttered and simpler
>
>  * we want to avoid writing things to the console, yet provide meaningful
> messages/codes to the developer
>
>  * we want HIGH performance no matter what
>
> One idea would be to discourage throwing exceptions in functions that we
> assume can be inlined due to these performance issues, but maybe propagate
> it more as Michael said throughout the code base, IF we agree that it
> simplifies our lives. Read more below...
>
>
> Btw, let's not forget that we're talking about the set of libraries in PCL
> and not the set of applications, higher level tools, etc. So question: what
> would other libraries do? :) STL? Boost? etc.
>
> We can also learn from Eigen. Eigen doesn't throw any exceptions. They use
> ASSERTS. They swear that "exceptions are definitely a costly feature for
> performance, whereas asserts are cheap". To quote even further "exceptions
> partly break the stack-based nature of the language, so they prevent the
> compiler from making optimizations".
>
>
> PS. Awesome topic! I was kidding about "vim" vs "emacs" though so please
> keep it friendly ;)
>
>
> Cheers,
> Radu.
> --
> Point Cloud Library (PCL) - http://pointclouds.org
>
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

nizar sallem
I vote for asserts whenever the error is unrecoverable (negative point
indices, negative length, etc.) and for exceptions instead of return
values cause I like the syntax try {cloud = loadPointCloud(file); }
catch (exception e) {"my bad something went wrong"}

On 09/22/2011 09:21 PM, Michael Dixon wrote:

> Thanks for the detailed reply!  I've got lots to think about now.  One
> quick response to the last point about "asserts":
>
> I think asserts might work in just about every case where I was
> imagining using exceptions.  They retain the benefits of not hijacking
> the return values, and they also don't force users to scatter error
> checking throughout all of their code.  The only problem is that they
> don't let you recover from an error.  Of course, lots of our errors
> are "unrecoverable" anyway.  If someone tries to pass bad inputs,
> terminating with a failed assertion makes perfect sense to me.  (Of
> course, we'll be screwing over anybody who may need to do some cleanup
> operations, like finishing writing out to a file, before exiting. But
> maybe that's okay.)
>
> The remaining question then is if we have "exceptional" cases where
> we'd like people to be able to catch them and continue running.  Those
> situations require either exceptions or return codes (or some
> as-of-yet-un-thought-of clever third alternative).  Can anybody think
> of some examples of when those situations (i.e. recoverable
> exceptions) could arise in PCL?
This is one: In super non linear ICP I try to register using some method
and when it fails (I get -1) I switch to another method cause I know
that if it fails then my system is ill formed. ;)

>
> -Michael
>
>
> On Thu, Sep 22, 2011 at 11:53 AM, Radu B. Rusu<[hidden email]>  wrote:
>> I knew we will get into polemics, that's a given with e-mails like this :)
>> Next on the list: "vim vs emacs" :)
>>
>>
>> So there's rules of thumb about "not abusing" exceptions, which could come
>> in handy. I usually inspire myself from:
>>
>>   * http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Exceptions
>>
>>   * Scott Meyers's wisdom that throwing an exception is about "three orders
>> of magnitude slower" than a normal return.
>>
>>   * POLA (Principle Of Least Astonishment) violations when it's simply not
>> obvious/straightforward what a method will do. If we use exceptions
>> everywhere, this can happen.
>>
>>   * Dave Abraham's http://www.boost.org/community/error_handling.html:
>>
>> "A more appropriate question to ask is: ``do we want stack unwinding here?''
>> Because actually handling an exception is likely to be significantly slower
>> than executing mainline code, you should also ask: ``Can I afford stack
>> unwinding here?'' For example, a desktop application performing a long
>> computation might periodically check to see whether the user had pressed a
>> cancel button. Throwing an exception could allow the operation to be
>> cancelled gracefully. On the other hand, it would probably be inappropriate
>> to throw and handle exceptions in the inner loop of this computation because
>> that could have a significant performance impact. The guideline mentioned
>> above has a grain of truth in it: in time critical code, throwing an
>> exception should be the exception, not the rule."
>>
>> Personally, I like Boost's way of doing things a lot (or I just got used to
>> it). The example that I can think of is polymorphic conversions via
>> dynamic_cast:
>> - there are cases where dynamic_cast produces an exception (std::bad_cast)
>> if the conversion is not possible when used on a reference type. Because
>> null references do not exist in C++, the conversion MUST succeed and result
>> in a valid reference, or it fails and you get an exception
>> - there are other cases where you want to convert pointer types and there it
>> simply returns a null pointer in case of an error
>>
>>   * inlining is a problem indeed. Try the following simple example:
>>
>>   1 #include<stdexcept>
>>   2 #include<iostream>
>>   3
>>   4 inline void
>>   5 foo ()
>>   6 {
>>   7   int a = 2;
>>   8   if (a == 2)
>>   9     throw new std::runtime_error ("test");
>>   10 }
>>   11
>>   12 inline void
>>   13 bar ()
>>   14 {
>>   15   int b = 4;
>>   16   if (b == 4)
>>   17     std::cerr<<  "test"<<  std::endl;
>>   18 }
>>   19
>>   20
>>   21 int main (int argc, char **argv)
>>   22 {
>>   23   bar ();
>>   24   foo ();
>>   25   return (0);
>>   26 }
>>
>> Compile with -s (-O1-3 not -O0 because that disables inlining completely)
>> and analyze the assembly output. You'll see an extra call in there for foo,
>> while the code for bar inlined nicely. Changing the type of the exception to
>> anything else doesn't seem to help -- just can't get rid of the extra "call
>> _Z3foov". So even without an exception specification, the compiler is
>> unhappy. And this is GCC -- we would like to support as many compilers as
>> possible, and inducing performance penalties like this stinks, and people
>> will hate us.
>>
>> [Edit: I tried the same with pathCC and ICC. Same result :(]
>>
>> Personally I think there is a good reason for this. Throwing an exception
>> invokes adding a new path of control flow to the program, which means it's
>> impossible for the compiler not to issue a long jump. This involves things
>> like pushing registers, jumping, returning and popping them, which is what
>> breaks performance.
>>
>> Of course there are probably "exceptions" to this too :)
>>
>>
>>   * another pitfall that is advertised almost everywhere is how to deal
>> within your code with functions that throw exceptions that call functions
>> that throw exceptions (phew), etc. You definitely do not want:
>>
>> try
>> {
>> ...
>> }
>> catch (...)
>> {
>> ...
>> }
>>
>>   * the reason why I mentioned we're not Java programmers, is that exceptions
>> in Java are actually more powerful as they are checked and enforced at
>> compile-time. Big difference.
>>
>>
>>
>>
>> This being said, we're not part of "the commonwealth". We're "unique snow
>> flowers", and adepts of "one hat size doesn't fit all". :) So forgetting
>> about what I said above (which is mostly cited from what others have said),
>> let's continue and see what we can find that's best for us.
>>
>>
>> Michael has a great point that we omitted discussing yet. In PCL 2.0 we
>> would like to use return types more, and be able to easily chain and
>> concatenate operators, pretty much like Eigen does (without the fancy lazy
>> evaluation :D).
>>
>>
>>
>> On 09/22/2011 10:07 AM, Michael Dixon wrote:
>>>
>>> I vote for using exceptions as our predominant error handling model.
>>> There are arguments for both sides, but here are the reasons I think
>>> exceptions are preferable:
>>>
>>> 1) Exceptions add a new channel of information, which let's you
>>> preserve a function's return value for more relevant data.  That is,
>>> if you use return codes, you can't use a function's return values for
>>> actual data/results.  So you have to abandon syntax like this:
>>>      PointCloud cloud = loadPointCloud ("file.pcd");
>>> in favor of:
>>>      PointCloud cloud;
>>>      int return_code = loadPointCloud ("file.pcd", cloud);
>>
>> This means that:
>>
>>>       PointCloud cloud = loadPointCloud ("file.pcd");
>>
>> +1
>>
>>>       PointCloud cloud;
>>>       int return_code = loadPointCloud ("file.pcd", cloud);
>>
>> -1
>>
>> Totally agreed.
>>
>>> 2) Exceptions can store lots of important information that can be used
>>> when handling an error, so the information needed to interpret the
>>> error and decide how to handle it is typically self-contained.  The
>>> exception can represent exactly why it was thrown and store an
>>> informative error message in case you want to display it or log it.
>>
>> Agreed. This votes for using something "like exceptions" as well.
>>
>>> 3) When you make people explicitly check return codes, they're forced
>>> to clutter up their code with a bunch of if-else's, which I think
>>> really hurts the readability.  Perhaps worse, people often just
>>> neglect to do any checking.  That's often fine... until they actually
>>> encounter an error.  Then instead of getting an obvious uncaught
>>> exception that explains to them exactly what went wrong and where,
>>> they just get weird behavior, and we get questions on pcl-users@.
>>
>> So this is an important distinction. If we agree that the output of a filter
>> which returns 0 points back (and thus filters everything) is not a CRITICAL
>> ERROR but rather an EXPECTED result from that operation, then the user has
>> the responsibility to check for it, and not us. If however the parameters
>> would be chosen in such a way that the filter would crash and produce -1
>> points (hehe :) ), then we would need to throw an exception, because it is
>> UNEXPECTED for the user to receive -1 points. :) Well, silly fictitious
>> example, but I hope it makes a point.
>>
>>
>>> I'm not familiar with the issue of exceptions preventing inlining.  Do
>>> you have any good resources I could read on this?  I tried to google
>>> it, and I didn't find any good explanation of it.  This discussion
>>> seems to indicate that throwing exceptions doesn't prevent inlining:
>>>
>>> http://stackoverflow.com/questions/7494399/would-c-exception-stops-function-from-being-inlined
>>> and this one:
>>>      http://www.gotw.ca/publications/mill22.htm
>>> mentions that "some compilers will automatically refuse to inline a
>>> function having an exception *specification*". But the solution to
>>> that problem is just to not use exception specifications (from the
>>> same article: "Moral #1: Never write an exception specification").
>>
>> See above. Also, stackoverflow has a lot of crappy answers and solutions
>> too, just between us. :) I also did a search and found silly things. The
>> problem is that they are never checked by experts, so people just vote by
>> themselves (like Wikipedia).
>>
>>> As for exceptions making code ugly?  I'd love for you to elaborate
>>> with an example, because I'm not sure I understand what you're getting
>>> at.  Personally, I think it's exactly the opposite.  Without
>>> exceptions, your code has to look like this:
>>
>> [...]
>>>
>>> This is pretty cluttered.  Plus, I really don't like putting
>>> statements (code that "does something") inside of "if" expressions,
>>> because I think it sort of hides them.  So I prefer to break it out
>>> even more, like this:
>>>
>>>      int return_code;
>>>
>>>      PointCloud cloud;
>>>      return_code = pcl::loadPointCloud ("file.pcd", cloud);
>>>      if (return_code == ERROR)
>>>        return ERROR;
>>>
>>>      PointCloud filtered;
>>>      return_code = pcl::doSomeFiltering (cloud, filtered);
>>>      if (return_code == ERROR)
>>>        return ERROR;
>>>
>>>      PointCloud features;
>>>      return_code = pcl::computeSomeFeatures (filtered, features);
>>>      if (return_code == ERROR)
>>>        return ERROR;
>>>
>>> but then your code just gets even more verbose.  On the other hand, if
>>> you use exceptions, the same operations can be expressed as:
>>>
>>>      PointCloud cloud = pcl::loadPointCloud ("file.pcd");
>>>      PointCloud filtered = pcl::doSomeFiltering (cloud);
>>>      PointCloud features = pcl::computeSomeFeatures (filtered);
>>
>> Hehe, this is not the same thing. This is: =)
>>
>> try
>> {
>>   PointCloud cloud = pcl::loadPointCloud ("file.pcd");
>> }
>> catch (ExceptionFoo)
>> {
>> ...
>> }
>> try
>> {
>>   PointCloud filtered = pcl::doSomeFiltering (cloud);
>> }
>> catch (ExceptionBar)
>> {
>> ...
>> }
>> try
>> {
>>   PointCloud features = pcl::computeSomeFeatures (filtered);
>> }
>> catch (ExceptionEps)
>> {
>> ...
>> }
>>
>> Or:
>>
>> try
>> {
>>   PointCloud cloud = pcl::loadPointCloud ("file.pcd");
>>   PointCloud filtered = pcl::doSomeFiltering (cloud);
>>   PointCloud features = pcl::computeSomeFeatures (filtered);
>> }
>> catch (ExceptionFoo)
>> {
>> ...
>> }
>> catch (ExceptionBar)
>> {
>> ...
>> }
>> catch (ExceptionEps)
>> {
>> ...
>> }
>> catch (...) /// ????? (do you even have a way of knowing them all?)
>>
>>
>>
>> What do others think? I personally like building a table with requirements.
>> Michael kick started 2.0 into this discussion which is really good. So:
>>
>>   * we want to use return types for data and not for error codes
>>
>>   * we want the code to be less cluttered and simpler
>>
>>   * we want to avoid writing things to the console, yet provide meaningful
>> messages/codes to the developer
>>
>>   * we want HIGH performance no matter what
>>
>> One idea would be to discourage throwing exceptions in functions that we
>> assume can be inlined due to these performance issues, but maybe propagate
>> it more as Michael said throughout the code base, IF we agree that it
>> simplifies our lives. Read more below...
>>
>>
>> Btw, let's not forget that we're talking about the set of libraries in PCL
>> and not the set of applications, higher level tools, etc. So question: what
>> would other libraries do? :) STL? Boost? etc.
>>
>> We can also learn from Eigen. Eigen doesn't throw any exceptions. They use
>> ASSERTS. They swear that "exceptions are definitely a costly feature for
>> performance, whereas asserts are cheap". To quote even further "exceptions
>> partly break the stack-based nature of the language, so they prevent the
>> compiler from making optimizations".
>>
>>
>> PS. Awesome topic! I was kidding about "vim" vs "emacs" though so please
>> keep it friendly ;)
>>
>>
>> Cheers,
>> Radu.
>> --
>> Point Cloud Library (PCL) - http://pointclouds.org
>>
> _______________________________________________
> PCL-developers mailing list
> [hidden email]
> http://pointclouds.org/mailman/listinfo/pcl-developers
> http://pointclouds.org
>
Cheers,
--
Nizar
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Brian Gerkey
On Thu, Sep 22, 2011 at 1:29 PM, nizar sallem <[hidden email]> wrote:
> I vote for asserts whenever the error is unrecoverable (negative point
> indices, negative length, etc.) and for exceptions instead of return values
> cause I like the syntax try {cloud = loadPointCloud(file); } catch
> (exception e) {"my bad something went wrong"}

One thing to note is that asserts are usually compiled out when
building in Release mode (full optimization).  Of course you don't
have to do this, but it is commonly done.  The idea is that asserts
are for the developers of the library, to protect against what ought
to be impossible situations during development, and that users never
see an assert.  Errors caused by user input (seems like Nizar's
negative index / length example is in this category, but I can't be
sure) is usually handled via exception or error code.  And I won't
weigh in on either side of that hot topic :)

        brian.
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Benoit Jacob
In reply to this post by Radu B. Rusu
> We can also learn from Eigen. Eigen doesn't throw any exceptions. They use
> ASSERTS. They swear that "exceptions are definitely a costly feature for
> performance, whereas asserts are cheap". To quote even further "exceptions
> partly break the stack-based nature of the language, so they prevent the
> compiler from making optimizations".

Indeed. Eigen doesn't throw any exception, except of course for
std::bad_alloc on memory allocation failure when C++ exceptions are
enabled.

For a good starting point for reading about the real performance cost
of C++ exceptions, check this:
http://www.codeproject.com/KB/cpp/cppexceptionsproetcontra.aspx#3_4

Benoit
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Radu B. Rusu
Administrator
Benoit,

Can you also weigh in please on your experience of catching asserts via SIGABRT? Michael's comments about asserts not
letting you recover from an error made me think that the only way to fix this is by trapping SIGABRT calls (which is
what abort() sends).


Cheers,
Radu.
--
Point Cloud Library (PCL) - http://pointclouds.org

On 09/22/2011 02:37 PM, Benoit Jacob wrote:

>> We can also learn from Eigen. Eigen doesn't throw any exceptions. They use
>> ASSERTS. They swear that "exceptions are definitely a costly feature for
>> performance, whereas asserts are cheap". To quote even further "exceptions
>> partly break the stack-based nature of the language, so they prevent the
>> compiler from making optimizations".
>
> Indeed. Eigen doesn't throw any exception, except of course for
> std::bad_alloc on memory allocation failure when C++ exceptions are
> enabled.
>
> For a good starting point for reading about the real performance cost
> of C++ exceptions, check this:
> http://www.codeproject.com/KB/cpp/cppexceptionsproetcontra.aspx#3_4
>
> Benoit
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Radu B. Rusu
Administrator
In reply to this post by Brian Gerkey
Brian,

On 09/22/2011 02:10 PM, Brian Gerkey wrote:

> On Thu, Sep 22, 2011 at 1:29 PM, nizar sallem<[hidden email]>  wrote:
>> I vote for asserts whenever the error is unrecoverable (negative point
>> indices, negative length, etc.) and for exceptions instead of return values
>> cause I like the syntax try {cloud = loadPointCloud(file); } catch
>> (exception e) {"my bad something went wrong"}
>
> One thing to note is that asserts are usually compiled out when
> building in Release mode (full optimization).  Of course you don't
> have to do this, but it is commonly done.  The idea is that asserts
> are for the developers of the library, to protect against what ought
> to be impossible situations during development, and that users never
> see an assert.  Errors caused by user input (seems like Nizar's
> negative index / length example is in this category, but I can't be
> sure) is usually handled via exception or error code.  And I won't
> weigh in on either side of that hot topic :)

Good point. Though while that's true, it's a minor issue. Looking at assert.h, they use simple #ifdef NDEBUG clauses. We
could easily extract the relevant assert calls from it if needed. Not a biggie, but I just wanted to mention that we can
bypass release vs debug if we need to.

Cheers,
Radu.
--
Point Cloud Library (PCL) - http://pointclouds.org

_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Benoit Jacob
In reply to this post by Radu B. Rusu
2011/9/22 Radu B. Rusu <[hidden email]>:
> Benoit,
>
> Can you also weigh in please on your experience of catching asserts via
> SIGABRT? Michael's comments about asserts not letting you recover from an
> error made me think that the only way to fix this is by trapping SIGABRT
> calls (which is what abort() sends).

Indeed, assert() signals SIGABRT and the only way to survive that is
to install a signal handler.

If you want a more flexible behavior, look at Mozilla's assertions.
First of all there is one thing about them that you should NOT copy:
by default they only print an error message and do not abort. This was
a terrible design choice. However there is one good thing about them:
their behavior can be tweaked at runtime by setting an environment
variable, XPCOM_DEBUG_BREAK.
   https://developer.mozilla.org/en/XPCOM_DEBUG_BREAK
By setting this env var to a suitable value, you can get assertion
failures to generate a SIGTRAP or a SIGSTOP instead. This is very
useful when running in a debugger, to break while allowing to
continue.

Cheers,
Benoit


>
>
> Cheers,
> Radu.
> --
> Point Cloud Library (PCL) - http://pointclouds.org
>
> On 09/22/2011 02:37 PM, Benoit Jacob wrote:
>>>
>>> We can also learn from Eigen. Eigen doesn't throw any exceptions. They
>>> use
>>> ASSERTS. They swear that "exceptions are definitely a costly feature for
>>> performance, whereas asserts are cheap". To quote even further
>>> "exceptions
>>> partly break the stack-based nature of the language, so they prevent the
>>> compiler from making optimizations".
>>
>> Indeed. Eigen doesn't throw any exception, except of course for
>> std::bad_alloc on memory allocation failure when C++ exceptions are
>> enabled.
>>
>> For a good starting point for reading about the real performance cost
>> of C++ exceptions, check this:
>> http://www.codeproject.com/KB/cpp/cppexceptionsproetcontra.aspx#3_4
>>
>> Benoit
>
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Radu B. Rusu
Administrator
In reply to this post by Michael Dixon-2
Oh, one thing that we still need to bring into the discussion, because it's related, is logging.

Logging is different than throw vs error code. Logging is good only for debugging but not for application developers.
Someone writing a GUI on top of PCL or ROS/ECTO nodes will not care whether and how we log, as they will not do string
parsing to provide their users with whatever error codes they need.

This means that we still need either ERROR CODES or EXCEPTIONS. Asserts might not function here because they are
booleans and I'm pretty sure that the only way to pass information back is via strings.

So to the list of requirements, again!:

* we want to use return types for data and not for error codes

ADDON here: Suat had a very nice idea about using the <<, >> operator instead of return types. We should discuss this as
well.

* we want the code to be less cluttered and simpler

* we want to avoid writing things to the console, yet provide meaningful messages/codes to the developer. This means
error codes or exceptions.

* we want HIGH performance no matter what, which means no exceptions thrown in inline-able methods.


Cheers,
Radu.
--
Point Cloud Library (PCL) - http://pointclouds.org

On 09/22/2011 12:21 PM, Michael Dixon wrote:

> Thanks for the detailed reply!  I've got lots to think about now.  One
> quick response to the last point about "asserts":
>
> I think asserts might work in just about every case where I was
> imagining using exceptions.  They retain the benefits of not hijacking
> the return values, and they also don't force users to scatter error
> checking throughout all of their code.  The only problem is that they
> don't let you recover from an error.  Of course, lots of our errors
> are "unrecoverable" anyway.  If someone tries to pass bad inputs,
> terminating with a failed assertion makes perfect sense to me.  (Of
> course, we'll be screwing over anybody who may need to do some cleanup
> operations, like finishing writing out to a file, before exiting. But
> maybe that's okay.)
>
> The remaining question then is if we have "exceptional" cases where
> we'd like people to be able to catch them and continue running.  Those
> situations require either exceptions or return codes (or some
> as-of-yet-un-thought-of clever third alternative).  Can anybody think
> of some examples of when those situations (i.e. recoverable
> exceptions) could arise in PCL?
>
> -Michael
>
>
> On Thu, Sep 22, 2011 at 11:53 AM, Radu B. Rusu<[hidden email]>  wrote:
>> I knew we will get into polemics, that's a given with e-mails like this :)
>> Next on the list: "vim vs emacs" :)
>>
>>
>> So there's rules of thumb about "not abusing" exceptions, which could come
>> in handy. I usually inspire myself from:
>>
>>   * http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Exceptions
>>
>>   * Scott Meyers's wisdom that throwing an exception is about "three orders
>> of magnitude slower" than a normal return.
>>
>>   * POLA (Principle Of Least Astonishment) violations when it's simply not
>> obvious/straightforward what a method will do. If we use exceptions
>> everywhere, this can happen.
>>
>>   * Dave Abraham's http://www.boost.org/community/error_handling.html:
>>
>> "A more appropriate question to ask is: ``do we want stack unwinding here?''
>> Because actually handling an exception is likely to be significantly slower
>> than executing mainline code, you should also ask: ``Can I afford stack
>> unwinding here?'' For example, a desktop application performing a long
>> computation might periodically check to see whether the user had pressed a
>> cancel button. Throwing an exception could allow the operation to be
>> cancelled gracefully. On the other hand, it would probably be inappropriate
>> to throw and handle exceptions in the inner loop of this computation because
>> that could have a significant performance impact. The guideline mentioned
>> above has a grain of truth in it: in time critical code, throwing an
>> exception should be the exception, not the rule."
>>
>> Personally, I like Boost's way of doing things a lot (or I just got used to
>> it). The example that I can think of is polymorphic conversions via
>> dynamic_cast:
>> - there are cases where dynamic_cast produces an exception (std::bad_cast)
>> if the conversion is not possible when used on a reference type. Because
>> null references do not exist in C++, the conversion MUST succeed and result
>> in a valid reference, or it fails and you get an exception
>> - there are other cases where you want to convert pointer types and there it
>> simply returns a null pointer in case of an error
>>
>>   * inlining is a problem indeed. Try the following simple example:
>>
>>   1 #include<stdexcept>
>>   2 #include<iostream>
>>   3
>>   4 inline void
>>   5 foo ()
>>   6 {
>>   7   int a = 2;
>>   8   if (a == 2)
>>   9     throw new std::runtime_error ("test");
>>   10 }
>>   11
>>   12 inline void
>>   13 bar ()
>>   14 {
>>   15   int b = 4;
>>   16   if (b == 4)
>>   17     std::cerr<<  "test"<<  std::endl;
>>   18 }
>>   19
>>   20
>>   21 int main (int argc, char **argv)
>>   22 {
>>   23   bar ();
>>   24   foo ();
>>   25   return (0);
>>   26 }
>>
>> Compile with -s (-O1-3 not -O0 because that disables inlining completely)
>> and analyze the assembly output. You'll see an extra call in there for foo,
>> while the code for bar inlined nicely. Changing the type of the exception to
>> anything else doesn't seem to help -- just can't get rid of the extra "call
>> _Z3foov". So even without an exception specification, the compiler is
>> unhappy. And this is GCC -- we would like to support as many compilers as
>> possible, and inducing performance penalties like this stinks, and people
>> will hate us.
>>
>> [Edit: I tried the same with pathCC and ICC. Same result :(]
>>
>> Personally I think there is a good reason for this. Throwing an exception
>> invokes adding a new path of control flow to the program, which means it's
>> impossible for the compiler not to issue a long jump. This involves things
>> like pushing registers, jumping, returning and popping them, which is what
>> breaks performance.
>>
>> Of course there are probably "exceptions" to this too :)
>>
>>
>>   * another pitfall that is advertised almost everywhere is how to deal
>> within your code with functions that throw exceptions that call functions
>> that throw exceptions (phew), etc. You definitely do not want:
>>
>> try
>> {
>> ...
>> }
>> catch (...)
>> {
>> ...
>> }
>>
>>   * the reason why I mentioned we're not Java programmers, is that exceptions
>> in Java are actually more powerful as they are checked and enforced at
>> compile-time. Big difference.
>>
>>
>>
>>
>> This being said, we're not part of "the commonwealth". We're "unique snow
>> flowers", and adepts of "one hat size doesn't fit all". :) So forgetting
>> about what I said above (which is mostly cited from what others have said),
>> let's continue and see what we can find that's best for us.
>>
>>
>> Michael has a great point that we omitted discussing yet. In PCL 2.0 we
>> would like to use return types more, and be able to easily chain and
>> concatenate operators, pretty much like Eigen does (without the fancy lazy
>> evaluation :D).
>>
>>
>>
>> On 09/22/2011 10:07 AM, Michael Dixon wrote:
>>>
>>> I vote for using exceptions as our predominant error handling model.
>>> There are arguments for both sides, but here are the reasons I think
>>> exceptions are preferable:
>>>
>>> 1) Exceptions add a new channel of information, which let's you
>>> preserve a function's return value for more relevant data.  That is,
>>> if you use return codes, you can't use a function's return values for
>>> actual data/results.  So you have to abandon syntax like this:
>>>      PointCloud cloud = loadPointCloud ("file.pcd");
>>> in favor of:
>>>      PointCloud cloud;
>>>      int return_code = loadPointCloud ("file.pcd", cloud);
>>
>> This means that:
>>
>>>       PointCloud cloud = loadPointCloud ("file.pcd");
>>
>> +1
>>
>>>       PointCloud cloud;
>>>       int return_code = loadPointCloud ("file.pcd", cloud);
>>
>> -1
>>
>> Totally agreed.
>>
>>> 2) Exceptions can store lots of important information that can be used
>>> when handling an error, so the information needed to interpret the
>>> error and decide how to handle it is typically self-contained.  The
>>> exception can represent exactly why it was thrown and store an
>>> informative error message in case you want to display it or log it.
>>
>> Agreed. This votes for using something "like exceptions" as well.
>>
>>> 3) When you make people explicitly check return codes, they're forced
>>> to clutter up their code with a bunch of if-else's, which I think
>>> really hurts the readability.  Perhaps worse, people often just
>>> neglect to do any checking.  That's often fine... until they actually
>>> encounter an error.  Then instead of getting an obvious uncaught
>>> exception that explains to them exactly what went wrong and where,
>>> they just get weird behavior, and we get questions on pcl-users@.
>>
>> So this is an important distinction. If we agree that the output of a filter
>> which returns 0 points back (and thus filters everything) is not a CRITICAL
>> ERROR but rather an EXPECTED result from that operation, then the user has
>> the responsibility to check for it, and not us. If however the parameters
>> would be chosen in such a way that the filter would crash and produce -1
>> points (hehe :) ), then we would need to throw an exception, because it is
>> UNEXPECTED for the user to receive -1 points. :) Well, silly fictitious
>> example, but I hope it makes a point.
>>
>>
>>> I'm not familiar with the issue of exceptions preventing inlining.  Do
>>> you have any good resources I could read on this?  I tried to google
>>> it, and I didn't find any good explanation of it.  This discussion
>>> seems to indicate that throwing exceptions doesn't prevent inlining:
>>>
>>> http://stackoverflow.com/questions/7494399/would-c-exception-stops-function-from-being-inlined
>>> and this one:
>>>      http://www.gotw.ca/publications/mill22.htm
>>> mentions that "some compilers will automatically refuse to inline a
>>> function having an exception *specification*". But the solution to
>>> that problem is just to not use exception specifications (from the
>>> same article: "Moral #1: Never write an exception specification").
>>
>> See above. Also, stackoverflow has a lot of crappy answers and solutions
>> too, just between us. :) I also did a search and found silly things. The
>> problem is that they are never checked by experts, so people just vote by
>> themselves (like Wikipedia).
>>
>>> As for exceptions making code ugly?  I'd love for you to elaborate
>>> with an example, because I'm not sure I understand what you're getting
>>> at.  Personally, I think it's exactly the opposite.  Without
>>> exceptions, your code has to look like this:
>>
>> [...]
>>>
>>> This is pretty cluttered.  Plus, I really don't like putting
>>> statements (code that "does something") inside of "if" expressions,
>>> because I think it sort of hides them.  So I prefer to break it out
>>> even more, like this:
>>>
>>>      int return_code;
>>>
>>>      PointCloud cloud;
>>>      return_code = pcl::loadPointCloud ("file.pcd", cloud);
>>>      if (return_code == ERROR)
>>>        return ERROR;
>>>
>>>      PointCloud filtered;
>>>      return_code = pcl::doSomeFiltering (cloud, filtered);
>>>      if (return_code == ERROR)
>>>        return ERROR;
>>>
>>>      PointCloud features;
>>>      return_code = pcl::computeSomeFeatures (filtered, features);
>>>      if (return_code == ERROR)
>>>        return ERROR;
>>>
>>> but then your code just gets even more verbose.  On the other hand, if
>>> you use exceptions, the same operations can be expressed as:
>>>
>>>      PointCloud cloud = pcl::loadPointCloud ("file.pcd");
>>>      PointCloud filtered = pcl::doSomeFiltering (cloud);
>>>      PointCloud features = pcl::computeSomeFeatures (filtered);
>>
>> Hehe, this is not the same thing. This is: =)
>>
>> try
>> {
>>   PointCloud cloud = pcl::loadPointCloud ("file.pcd");
>> }
>> catch (ExceptionFoo)
>> {
>> ...
>> }
>> try
>> {
>>   PointCloud filtered = pcl::doSomeFiltering (cloud);
>> }
>> catch (ExceptionBar)
>> {
>> ...
>> }
>> try
>> {
>>   PointCloud features = pcl::computeSomeFeatures (filtered);
>> }
>> catch (ExceptionEps)
>> {
>> ...
>> }
>>
>> Or:
>>
>> try
>> {
>>   PointCloud cloud = pcl::loadPointCloud ("file.pcd");
>>   PointCloud filtered = pcl::doSomeFiltering (cloud);
>>   PointCloud features = pcl::computeSomeFeatures (filtered);
>> }
>> catch (ExceptionFoo)
>> {
>> ...
>> }
>> catch (ExceptionBar)
>> {
>> ...
>> }
>> catch (ExceptionEps)
>> {
>> ...
>> }
>> catch (...) /// ????? (do you even have a way of knowing them all?)
>>
>>
>>
>> What do others think? I personally like building a table with requirements.
>> Michael kick started 2.0 into this discussion which is really good. So:
>>
>>   * we want to use return types for data and not for error codes
>>
>>   * we want the code to be less cluttered and simpler
>>
>>   * we want to avoid writing things to the console, yet provide meaningful
>> messages/codes to the developer
>>
>>   * we want HIGH performance no matter what
>>
>> One idea would be to discourage throwing exceptions in functions that we
>> assume can be inlined due to these performance issues, but maybe propagate
>> it more as Michael said throughout the code base, IF we agree that it
>> simplifies our lives. Read more below...
>>
>>
>> Btw, let's not forget that we're talking about the set of libraries in PCL
>> and not the set of applications, higher level tools, etc. So question: what
>> would other libraries do? :) STL? Boost? etc.
>>
>> We can also learn from Eigen. Eigen doesn't throw any exceptions. They use
>> ASSERTS. They swear that "exceptions are definitely a costly feature for
>> performance, whereas asserts are cheap". To quote even further "exceptions
>> partly break the stack-based nature of the language, so they prevent the
>> compiler from making optimizations".
>>
>>
>> PS. Awesome topic! I was kidding about "vim" vs "emacs" though so please
>> keep it friendly ;)
>>
>>
>> Cheers,
>> Radu.
>> --
>> Point Cloud Library (PCL) - http://pointclouds.org
>>
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Radu B. Rusu
Administrator
In reply to this post by Benoit Jacob
Benoit,


On 09/22/2011 04:40 PM, Benoit Jacob wrote:

> 2011/9/22 Radu B. Rusu<[hidden email]>:
>> Benoit,
>>
>> Can you also weigh in please on your experience of catching asserts via
>> SIGABRT? Michael's comments about asserts not letting you recover from an
>> error made me think that the only way to fix this is by trapping SIGABRT
>> calls (which is what abort() sends).
>
> Indeed, assert() signals SIGABRT and the only way to survive that is
> to install a signal handler.
>
> If you want a more flexible behavior, look at Mozilla's assertions.
> First of all there is one thing about them that you should NOT copy:
> by default they only print an error message and do not abort. This was
> a terrible design choice. However there is one good thing about them:
> their behavior can be tweaked at runtime by setting an environment
> variable, XPCOM_DEBUG_BREAK.
>     https://developer.mozilla.org/en/XPCOM_DEBUG_BREAK
> By setting this env var to a suitable value, you can get assertion
> failures to generate a SIGTRAP or a SIGSTOP instead. This is very
> useful when running in a debugger, to break while allowing to
> continue.

Interesting. Thanks for sharing!

Did you manage to pass anything else other than strings using asserts? One worry (see my previous e-mail) is that they
are only valid for the developer. An application built on top still requires certain error codes or exceptions when
things go bad, so that it can convert them into meaningful messages/actions to the user.


Cheers,
Radu.
--
Point Cloud Library (PCL) - http://pointclouds.org
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Brian Gerkey
In reply to this post by Radu B. Rusu
On Thu, Sep 22, 2011 at 4:36 PM, Radu B. Rusu <[hidden email]> wrote:

> On 09/22/2011 02:10 PM, Brian Gerkey wrote:
>> One thing to note is that asserts are usually compiled out when
>> building in Release mode (full optimization).  Of course you don't
>> have to do this, but it is commonly done.  The idea is that asserts
>> are for the developers of the library, to protect against what ought
>> to be impossible situations during development, and that users never
>> see an assert.  Errors caused by user input (seems like Nizar's
>> negative index / length example is in this category, but I can't be
>> sure) is usually handled via exception or error code.  And I won't
>> weigh in on either side of that hot topic :)
>
> Good point. Though while that's true, it's a minor issue. Looking at
> assert.h, they use simple #ifdef NDEBUG clauses. We could easily extract the
> relevant assert calls from it if needed. Not a biggie, but I just wanted to
> mention that we can bypass release vs debug if we need to.

Indeed, it's easy to work around.  We did this in rosbuild for exactly
the reason that we wanted optimized releases but needed asserts
enabled, because developers had assumed that they would always be
there.  But we had to define a new build type that CMake didn't know
about: RelWithAsserts
(http://www.ros.org/wiki/rosbuild#Customizing_the_build_.28debug.2C_optimizations.2C_default_build_flags.29).

I brought this up mainly to point out that it's common practice to
compile out asserts (as evidenced by CMake's lack of a standard
configuration the combines optimization with asserts).  You definitely
don't have to follow that practice (maybe it's outdated?), but it's
good to be aware of it, as users may expect different behavior (or may
induce different behavior by, for example, compiling against PCL with
-DNDEBUG, disabling asserts that live in headers, if there are any).

        brian.
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Benoit Jacob
In reply to this post by Radu B. Rusu
2011/9/22 Radu B. Rusu <[hidden email]>:

> Benoit,
>
>
> On 09/22/2011 04:40 PM, Benoit Jacob wrote:
>>
>> 2011/9/22 Radu B. Rusu<[hidden email]>:
>>>
>>> Benoit,
>>>
>>> Can you also weigh in please on your experience of catching asserts via
>>> SIGABRT? Michael's comments about asserts not letting you recover from an
>>> error made me think that the only way to fix this is by trapping SIGABRT
>>> calls (which is what abort() sends).
>>
>> Indeed, assert() signals SIGABRT and the only way to survive that is
>> to install a signal handler.
>>
>> If you want a more flexible behavior, look at Mozilla's assertions.
>> First of all there is one thing about them that you should NOT copy:
>> by default they only print an error message and do not abort. This was
>> a terrible design choice. However there is one good thing about them:
>> their behavior can be tweaked at runtime by setting an environment
>> variable, XPCOM_DEBUG_BREAK.
>>    https://developer.mozilla.org/en/XPCOM_DEBUG_BREAK
>> By setting this env var to a suitable value, you can get assertion
>> failures to generate a SIGTRAP or a SIGSTOP instead. This is very
>> useful when running in a debugger, to break while allowing to
>> continue.
>
> Interesting. Thanks for sharing!
>
> Did you manage to pass anything else other than strings using asserts? One
> worry (see my previous e-mail) is that they are only valid for the
> developer. An application built on top still requires certain error codes or
> exceptions when things go bad, so that it can convert them into meaningful
> messages/actions to the user.

Assertions are useful to guard against plain programming mistakes that
will lead to something as bad as a crash. The canonical example is
out-of-bounds accesses into arrays, usage of arrays of mismatched
sizes, etc. That's how Eigen uses asserts. Assertions help there
because they allow to get a good clean crash right away, rather than
continue with undefined behavior. They are a debugging helper.

So if you want to report an error to a user, clearly assertions are
not the right tool.

Benoit



>
>
> Cheers,
> Radu.
> --
> Point Cloud Library (PCL) - http://pointclouds.org
>
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Geoffrey Biggs
Wow, it seems I fell asleep and came late to the party!

For what it's worth, I'll through in a few points.

First, going way back to Radu's original email and the first reply:

On 22/09/11 17:49, Jochen Sprickerhof wrote:
 > * Radu Rusu<[hidden email]>  [2011-09-21 19:55]:
 >> We also need to work to make PCL_XXX redirect to user given
 >> ostreams. I like this pattern a lot:
 >>
 >>
https://github.com/gbiggs/hokuyoaist/blob/master/include/hokuyoaist/sensor.h,
line 134.
 >>
 >> If a user wants output to the console, they pass cout/cerr/clog to
 >> the constructor. Clean, efficient, simple.
 >
 > Veto. We should not make assumptions on what application developers
 > use. Maybe they want to localize the message or feed it into a GUI
 > and that would be really hard if it's a stream already.  Basically I
 > would like to get rid of all PCL_XXX and cout stuff because it's
 > carrying information without providing easy access to it.  I haven't
 > investigated the alternatives, but one way would be to provide custom
 > exceptions that encapsulate the message.

Jochen, I think you're making a false assumption. The intention is not
that the user must pass one of the STL stream implementations;
std::ostream is an interface. The intention is that the user will
provide their own implementation of std::ostream that meets their needs,
if an existing implementation does not. So, to use your example, they
would implement a class inheriting from std::ostream that would receive
the debug output and display it in the GUI. It is important to remember
that, despite being called a stream, the interface is actually discrete.
Thus, this pattern does provide easy access to the debug output, and it
can be funneled to a null implementation when not needed, which should
be the default.

I personally don't believe that libraries should use logging facilities.
That is for applications. Libraries, if they provide logging information
at all, should only provide an interface to get it for the application
to use to funnel it to their desired destination. Ideally, logging
libraries like log4cxx would provide a std::ostream interface.


Now for some general points:

1. Asserts have been mentioned. Asserts are *absolute*. They are a
guaranteed failure when something has gone *really* wrong internally.
What matters for asserts is what we define as "internal." If we define
the boundary as being the PCL API, then ideally users of PCL (that means
developers using it in their own applications - end users of those
applications shouldn't ever see an error direct from PCL) shouldn't even
know they're in PCL, because if they see an assert then it means PCL has
suffered an *internal* error. *External* errors should not be checked by
asserts because they need to give feedback to whatever/whomever is
beyond the boundary between internal an external.

So the most important question for using asserts is, where is our
boundary? Is it the PCL API or do we consider that developers working
with PCL are within it as well?

Either way, we shouldn't be playing with the macros that control them.
We should be asking anyone within our boundary to compile in debug mode
until they are sure their code is correct. I understand this will cause
problems for users working with binaries, so perhaps we need debug
binaries as well if our immediate users are within the boundary.

2. Functions should only ever return expected values.

3. Exceptions are exactly what they mean: exceptional situations. This
means that the situation encountered is assumed to be bad enough as to
be unrecoverable without some severe action. As Radu mentioned in an
earlier email, stack unwinding is involved and it is rather inefficient.
Based on this reasoning, we would say that exceptions should only be
used in situations where the error is bad enough to warrant the time
spent cleaning it up.


In the Erlang world we have some fairly good guidelines about when to
use exceptions and when to use return values. They can't be applied
directly because Erlang has different efficiency requirements and a
dynamic typing system, but here they are anyway:

- Return values must only ever contain expected values. For example, if
I have a function that should return a large data structure, it can't
return "error" on failure (error is a legitimate value in Erlang, it's
called an atom). This is because the developer may not check if the
value returned was the correct data structure, and later on they'll get
another, seemingly unrelated exception when they try to use that value
as a data structure that it isn't. It is very difficult to back track
from this exception to the real cause: a failed function.

- Fail early. As early as possible. (This is helped in Erlang by the VM
itself throwing exceptions in certain, useful situations.) Exceptions
are for exceptional situations. Therefore, the program cannot continue
normal operation and so should begin recovery procedures as soon as
possible rather than wasting time. (In Erlang, this usually means
killing a bunch of processes and letting their supervisors re-create
them.) When you fail, record as much information about the error as
possible so that it can be debugged properly later - not straight away!
You will get bug reports from clients/users, you need to be able to
debug them.

- Assume, and ensure, that your program is correct internally. Use
asserts to achieve this and test properly so that the asserts can be
removed in a release build. In tandem with this, check all input at the
edge. If you guarantee that the input is correct and your program is
correct, then you will not have any failures except at the edge.
Obviously your program won't be 100% correct, but that's what the
asserts and proper testing are for. :) Where this point would be applied
in PCL comes back to defining where the boundary is.

OK, I think I've wasted enough electrons and photons now.
Geoff
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Andreas Mützel
Thanks for posting this just as I was writing my answer. That's basically
exactly what I wanted to write, except for the erlang reference ;-)

For the boundary, I would vote to set it at the public PCL API and to use
assertions for all internal stuff, i.e. to check preconditions and maybe
postconditions of functions that are supposed to be called only by other PCL
functions.

So, for example:
In the copyPointCloud case mentioned somewhere (input==output),an exception
should be thrown. But things like private member functions of the KdTree class
should use assertions, as all data that reaches them has been passed via the
public functions. IMHO, these public functions would be the right place to
check for invalid conditions because of the "fail early" principle.

Of course, there are cases that aren't as clear as those, but as a general
guideline, I totally agree with what Geoffrey wrote.
Personally, I would consider basing the decision on performance considerations
a premature optimization. Recovering from errors isn't something that should
be done often, and I guess the whole recovery is likely to take much longer
than throwing/catching the exception.
(I wouldn't worry about the no-inlining thing, unless the error checking is
done in an inner loop and real performance impacts can be demonstrated.)

Cheers,
Andreas

On Friday 23 September 2011 11:57:42 Geoffrey Biggs wrote:

> Now for some general points:
>
> 1. Asserts have been mentioned. Asserts are *absolute*. They are a
> guaranteed failure when something has gone *really* wrong internally.
> What matters for asserts is what we define as "internal." If we define
> the boundary as being the PCL API, then ideally users of PCL (that means
> developers using it in their own applications - end users of those
> applications shouldn't ever see an error direct from PCL) shouldn't even
> know they're in PCL, because if they see an assert then it means PCL has
> suffered an *internal* error. *External* errors should not be checked by
> asserts because they need to give feedback to whatever/whomever is
> beyond the boundary between internal an external.
>
> So the most important question for using asserts is, where is our
> boundary? Is it the PCL API or do we consider that developers working
> with PCL are within it as well?
>
> Either way, we shouldn't be playing with the macros that control them.
> We should be asking anyone within our boundary to compile in debug mode
> until they are sure their code is correct. I understand this will cause
> problems for users working with binaries, so perhaps we need debug
> binaries as well if our immediate users are within the boundary.
>
> 2. Functions should only ever return expected values.
>
> 3. Exceptions are exactly what they mean: exceptional situations. This
> means that the situation encountered is assumed to be bad enough as to
> be unrecoverable without some severe action. As Radu mentioned in an
> earlier email, stack unwinding is involved and it is rather inefficient.
> Based on this reasoning, we would say that exceptions should only be
> used in situations where the error is bad enough to warrant the time
> spent cleaning it up.
>
>
> In the Erlang world we have some fairly good guidelines about when to
> use exceptions and when to use return values. They can't be applied
> directly because Erlang has different efficiency requirements and a
> dynamic typing system, but here they are anyway:
>
> - Return values must only ever contain expected values. For example, if
> I have a function that should return a large data structure, it can't
> return "error" on failure (error is a legitimate value in Erlang, it's
> called an atom). This is because the developer may not check if the
> value returned was the correct data structure, and later on they'll get
> another, seemingly unrelated exception when they try to use that value
> as a data structure that it isn't. It is very difficult to back track
> from this exception to the real cause: a failed function.
>
> - Fail early. As early as possible. (This is helped in Erlang by the VM
> itself throwing exceptions in certain, useful situations.) Exceptions
> are for exceptional situations. Therefore, the program cannot continue
> normal operation and so should begin recovery procedures as soon as
> possible rather than wasting time. (In Erlang, this usually means
> killing a bunch of processes and letting their supervisors re-create
> them.) When you fail, record as much information about the error as
> possible so that it can be debugged properly later - not straight away!
> You will get bug reports from clients/users, you need to be able to
> debug them.
>
> - Assume, and ensure, that your program is correct internally. Use
> asserts to achieve this and test properly so that the asserts can be
> removed in a release build. In tandem with this, check all input at the
> edge. If you guarantee that the input is correct and your program is
> correct, then you will not have any failures except at the edge.
> Obviously your program won't be 100% correct, but that's what the
> asserts and proper testing are for. :) Where this point would be applied
> in PCL comes back to defining where the boundary is.
>
> OK, I think I've wasted enough electrons and photons now.
> Geoff
> _______________________________________________
> PCL-developers mailing list
> [hidden email]
> http://pointclouds.org/mailman/listinfo/pcl-developers
> http://pointclouds.org
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
Reply | Threaded
Open this post in threaded view
|

Re: to throw or not to throw

Jochen Sprickerhof
Administrator
In reply to this post by Geoffrey Biggs
Hi Geoffrey,

* Geoffrey Biggs <[hidden email]> [2011-09-23 18:57]:

> Wow, it seems I fell asleep and came late to the party!
>
> For what it's worth, I'll through in a few points.
>
> First, going way back to Radu's original email and the first reply:
>
> On 22/09/11 17:49, Jochen Sprickerhof wrote:
> > * Radu Rusu<[hidden email]>  [2011-09-21 19:55]:
> >> We also need to work to make PCL_XXX redirect to user given
> >> ostreams. I like this pattern a lot:
> >>
> >> https://github.com/gbiggs/hokuyoaist/blob/master/include/hokuyoaist/sensor.h,
> line 134.
> >>
> >> If a user wants output to the console, they pass cout/cerr/clog to
> >> the constructor. Clean, efficient, simple.
> >
> > Veto. We should not make assumptions on what application developers
> > use. Maybe they want to localize the message or feed it into a GUI
> > and that would be really hard if it's a stream already.  Basically I
> > would like to get rid of all PCL_XXX and cout stuff because it's
> > carrying information without providing easy access to it.  I haven't
> > investigated the alternatives, but one way would be to provide custom
> > exceptions that encapsulate the message.
>
> Jochen, I think you're making a false assumption. The intention is
> not that the user must pass one of the STL stream implementations;
> std::ostream is an interface. The intention is that the user will
> provide their own implementation of std::ostream that meets their
> needs, if an existing implementation does not. So, to use your
> example, they would implement a class inheriting from std::ostream
> that would receive the debug output and display it in the GUI. It is
> important to remember that, despite being called a stream, the
> interface is actually discrete. Thus, this pattern does provide easy
> access to the debug output, and it can be funneled to a null
> implementation when not needed, which should be the default.

just because you addressed me personally. You are totally right that
it's just an interface, but that wasn't my point. The problem is that
you as a developer using PCL will have a hard time identifying the error
and handling it in a different way, when you only have this interface.
Compare it for example with error handling in C (see perror and
strerror, which would be roughly the same as the ostream interface).

Cheers,

Jochen
_______________________________________________
PCL-developers mailing list
[hidden email]
http://pointclouds.org/mailman/listinfo/pcl-developers
http://pointclouds.org
123