c++ - C++ , is this goto statement warranted?

Question

I have changed title slightly because I thought this is more appropriate question.

Would you refactor it (seems like legitimate use of goto) ? If, how would you refactor the following code to remove go to statement?

if (data.device) {
    try {
        ...
    }
    catch(const std::exception&) { goto done; }
    ... // more things which should not be caught
done: ;
}

complete statement

#ifdef HAVE_GPU
            // attempt to use GPU device
            if (data.device) {
                try {
                    Integral::Gpu eri(S, R, Q, block.shell());
                    eri(basis.centers(), quartets, data.device);
                }
                // if GPU fails, propagate to cpu
                catch(std::exception) { goto done; }
                data.device += size;
                host_index.extend(block_index);
                block_index.data.clear();
            done: ;
            }
#endif

thank you

after have seen preference of most, I decided to go with flag, but with Mr. York comment.

Thank you everybody

score 15 · Accepted Answer

if (data.device)
{
    bool status = true;

    try
    {
        ...
    }
    catch(std::exception)
    {
        status = false;
    }

    if (status)
    {
    ... // more things which should not be caught
    }
}

score 8 · Accepted Answer

First: goto isn't evil per se. Refactoring for the pure sake of not having the letters "goto" in your code is nonsense. Refactoring it to something which gets the same thing done cleaner than a goto is fine. Changing a bad design into one which is better and doesn't need a goto or a replacement for it is fine, too.

That being said, I'd say your code looks exactly like what finally was invented for. Too sad C++ doesn't have something like this... so maybe the easiest solution is to leave it like that.

score 7 · Accepted Answer

You can catch the exception and rethrow a specific exception that can be handled outside of the conditional block.

// obviously you would want to name this appropriately...
struct specific_exception : std::exception { };

try {
    if (data.device) {
        try {
            // ...
        }
        catch(const std::exception&) { 
            throw specific_exception(); 
        }

        // ... more things which should not be caught ...
    }
}
catch (const specific_exception&) { 
    // handle exception
}

score 4 · Accepted Answer

Why not move the extra code inside the try block?:

#ifdef HAVE_GPU
            // attempt to use GPU device
            if (data.device) {
                try {
                    Integral::Gpu eri(S, R, Q, block.shell());
                    eri(basis.centers(), quartets, data.device);
                    data.device += size;
                    host_index.extend(block_index);
                    block_index.data.clear();
                }
                // if GPU fails, propagate to cpu
                catch(std::exception) { /* handle your exception */; }
            }
#endif

score 4 · Accepted Answer

I think a variant of this might work for you.

// attempt to use GPU device
if (data.device)
{
    try
    {
        Integral::Gpu eri(S, R, Q, block.shell());
        eri(basis.centers(), quartets, data.device);

        data.device += size;
        host_index.extend(block_index);
        block_index.data.clear();
    }
    catch (const std::bad_alloc&)
    {
        // this failure was not because 
        // of the GPU, let it propagate
        throw;
    }
    catch(...)
    {
        // handle any other exceptions by
        // knowing it was the GPU and we 
        // can fall back onto the CPU.
    }
}

// do CPU

If you could edit the GPU library and give all GPU exceptions some base like gpu_exception, the code becomes much simpler:

// attempt to use GPU device
if (data.device)
{
    try
    {
        Integral::Gpu eri(S, R, Q, block.shell());
        eri(basis.centers(), quartets, data.device);

        data.device += size;
        host_index.extend(block_index);
        block_index.data.clear();
    }
    catch (const gpu_exception&)
    {
        // handle GPU exceptions by
        // doing nothing and falling
        // back onto the CPU.
    }

    // all other exceptions, not being 
    // GPU caused, may propagate normally
}

// do CPU

If nether of these work, I think the next best thing is Steve's answer.

score 4 · Accepted Answer

Slightly different use of a flag. I think it's neater than Amardeep's.

I'd rather use a flag to indicate whether to propagate the exception, than a flag to indicate whether the last thing worked, because the entire point of exceptions is to avoid checks for whether the last thing worked -- the idea is to write code such that if we got this far, then everything worked and we're good to continue.

#ifdef HAVE_GPU
    // attempt to use GPU device
    if (data.device) {
        bool dont_catch = false;
        try {
            ...
            dont_catch = true;
            ... // more things which should not be caught
        } catch (...) {
            if (dont_catch) throw;
        }
    }
#endif

score 3 · Accepted Answer

if (data.device) {
    bool ok = true;
    try {
        ...
    }
    catch(std::exception) { ok = false; }

    if(ok) {
        ... // more things which should not be caught
    }
}

score 2 · Accepted Answer

2

Can you not catch the exception outside the if?

于 2010-07-27T17:03:51.753 回答

score 2 · Accepted Answer

When there is a bunch of code that needs to exit based on some condition, my preferred construct is to use a "do {} while(0)" loop and "break" when appropriate. I don't know what break; will do in a catch, though. A "goto" might be your best bet if "break" won't work.

score 2 · Accepted Answer

Am I missing something, or wouldn't it be equivalent to move the part between the catch and done: label inside the try block?

#ifdef HAVE_GPU
            // attempt to use GPU device
            if (data.device) {
                try {
                    Integral::Gpu eri(S, R, Q, block.shell());
                    eri(basis.centers(), quartets, data.device);
                    data.device += size;
                    host_index.extend(block_index);
                    block_index.data.clear();
                }
                // if GPU fails, propagate to cpu
                catch(std::exception) {}
            }
#endif

score 1 · Accepted Answer

What about just using some flag and adding a conditional statement?

int caught = 0;
if (data.device) {
    try {
        /* ... */
    } catch (std::exception e) { caught = 1; }
    if (!caught) {
        // more stuff here
    }
    // done: ...
}

score 1 · Accepted Answer

catch(std::exception) { return; }

That should do the trick. I am, of course, assuming that done is indeed at the end of a function.

If you need to execute additional code when the exception occurs, return a status or throw an exception that is at a more proper level of abstraction (see James' answer).

I'm imagining something like:

doStuff(...) {
    bool gpuSuccess = doGPUStuff(...);
    if (!gpuSuccess) {
        doCPUStuff(...);
    }
}

score 1 · Accepted Answer

Just break it out into its own function/method (including everything after it) and use the return keyword. As long as all your variables have destructors, are stack-allocated (or smart-pointered if unavoidable), then you're fine. I'm a big fan of early-exit functions/methods rather than continual flag-checking.

for example:

void myFunc()
{
    String mystr("heya");
    try
    {
        couldThrow(mystr);
    }
    catch(MyException& ex)
    {
        return; // String mystr is freed upon returning
    }

    // Only execute here if no exceptions above
    doStuff();
}

This way, it's hard to go wrong

score 1 · Accepted Answer

The reason goto is frowned on today is that we have these fancy things called "functions" instead. Wrap the GPU code in its own function, which can return early if it fails.

Then call that from your original function.

Since they'll probably need to share a few variables (data, host_index and block_index, it looks like), put them in a class, and make the two functions members of it.

void RunOnGpu(){
        // attempt to use GPU device
        if (data.device) {
            try {
                Integral::Gpu eri(S, R, Q, block.shell());
                eri(basis.centers(), quartets, data.device);
            }
            // if GPU fails, propagate to cpu
            catch(std::exception) { return; }
            data.device += size;
            host_index.extend(block_index);
            block_index.data.clear();     
}
void DoStuff() {
#ifdef HAVE_GPU
    RunOnGpu();
#endif
}

c++ - C++ , is this goto statement warranted?

14 回答 14

Related

Reference