patrikhuber / eos-model-viewer

3D model viewer for the eos Morphable Model library
Apache License 2.0
144 stars 53 forks source link

Catch exceptions in main()? #11

Closed elfring closed 6 years ago

elfring commented 6 years ago

I expect that exception handling is usually supported by a C++ program. I wonder why your function “main” does not contain a catch-all handler so far.

How do you think about recommendations by Matthew Wilson in an article?

Would you like to adjust the implementation if you consider effects for uncaught/unhandled exceptions?

patrikhuber commented 6 years ago

Hi,

As noted in the readme, the viewer is seriously unpolished code, it's a quickly hacked-together thing that I use for myself internally and that I thought I make available online because it could be useful to others, even in its "hacky" state.

Note: The viewer works well, but the code is not very polished and will crash if you do unexpected things (e.g. cancel the loading dialogues). The CMake scripts are in serious alpha-stage - You are on your own compiling it!

However, it's great if we can improve it. Where do you want to add the try/catch? Presumably around viewer.launch();? Do you think a catch-all (...) is a good idea? Isn't there a guideline that says we should only catch the exceptions that we can actually handle, and pass the rest on to the callee? Or does that guideline not apply to main()?

Thanks & feel free to open a PR!

elfring commented 6 years ago

Do you think a catch-all (...) is a good idea?

The available information sources can provide incentives for software design adjustments, can't they?

patrikhuber commented 6 years ago

The available information sources can provide incentives for software design adjustments, can't they?

They can, but there's also tons of old, outdated and wrong information out there. Finding out which source is "good", reliable or up-to-date is often considerable effort.

For example, your link suggests this:

#include <iostream>
int main()
{

    try
    {
        runGame();
    }
    catch(...)
    {
        std::cerr << "Abnormal termination\n";
    }

    saveState(); // Save user's game
    return 1;
}

From what I have learned and from my experience (which could very possibly be wrong here!), I would rather do the following:

#include <iostream>
int main()
{

    try
    {
        runGame();
    }
    catch(...)
    {
        std::cerr << "Abnormal termination\n";
        saveState(); // Save user's game
        throw; // re-throw the exception to the callee - not 100% sure that's how you do it
    }
    return 0;
}

Does what I think make sense? Which one is better? Sure it could be researched, it would take me a couple of hours or so, and I am a bit of a perfectionist in general, but for this model-viewer, currently, it is not worth it for me personally. But I think it sounds like your C++ knowledge/experience is much bigger than mine, so if you already know the best practice in this case, I would be very happy to accept a PR! :-)

elfring commented 6 years ago

But I think it sounds like your C++ knowledge/experience is much bigger than mine, …

Such an impression can distract from an other solution.

patrikhuber commented 6 years ago

Does your evolving model viewer manage any data which should be safely stored (at the end)?

No, it doesn't modify anything at all. It can just be quit and if it crashes then it doesn't matter either.

Have you got any software development opinions for the following information?

I'm not actually sure I understand it, I'm not sure why it talks about try-blocks and not catch-blocks. I'd have to read the whole page in detail and get a bit more familiar with the context.

elfring commented 6 years ago

No, it doesn't modify anything at all.

This can be fine for a simple data viewer.

I assumed from a glimpse at the published screenshot that display parameters could be adjusted. (The draw options and coefficients did not look as greyed-out/read-only settings in the graphical user interface.)

patrikhuber commented 6 years ago

Sure these can be adjusted, but they are not really too interesting to persist them. Of course, if this was a production-quality tool, then one would persist some of these settings.

I'm actually not sure what this is all about. Are you using the model-viewer and would like to potentially contribute to it?

elfring commented 6 years ago

I checked only the general change acceptance for further software evolution so far.

patrikhuber commented 6 years ago

I checked only the general change acceptance for further software evolution so far.

What? I'm sorry but this sentence does not make too much sense to me.

I don't think we're getting anywhere here. Actually, upon further inspection of your GitHub profile, I found that you posted a very similar issue in dozens of other GitHub repositories over the last couple of days, and you very often reply with the same, apparently pre-written comment messages. I'm really not sure what your goal here is, but I am closing this for now, until there's some substantial reason to re-open this or if you are really interested in contributing.

elfring commented 6 years ago

I identified the possibility for another software change around the implementation also of this model viewer. :crystal_ball: It can become interesting then under which circumstances possible changes will be integrated somehow, can't it?