CODEWITHSUNDEEP
c++exceptiongoto
Anycorn
Anycorn

Reputation: 51465

C++ , is this goto statement warranted?

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

Upvotes: 9

Views: 1285

Answers (14)

Stack Overflow is garbage
Stack Overflow is garbage

Reputation: 247949

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
}

Upvotes: 1

Justin Ardini
Justin Ardini

Reputation: 9866

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(...);
    }
}

Upvotes: 1

Kevin Anderson
Kevin Anderson

Reputation: 7010

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

Upvotes: 1

GManNickG
GManNickG

Reputation: 503835

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.

Upvotes: 4

Nick Meyer
Nick Meyer

Reputation: 40272

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

Upvotes: 2

Steve Jessop
Steve Jessop

Reputation: 279255

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

Upvotes: 4

Jeff Paquette
Jeff Paquette

Reputation: 7127

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

Upvotes: 4

James McNellis
James McNellis

Reputation: 355049

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
}

Upvotes: 7

supercat
supercat

Reputation: 81149

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.

Upvotes: 2

user395760
user395760

Reputation:

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.

Upvotes: 8

TJ Koblentz
TJ Koblentz

Reputation: 6948

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: ...
}

Upvotes: 1

Scott Saunders
Scott Saunders

Reputation: 30394

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

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

Upvotes: 3

Amardeep AC9MF
Amardeep AC9MF

Reputation: 19044

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

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

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

Upvotes: 15

djna
djna

Reputation: 55907

Can you not catch the exception outside the if?

Upvotes: 2

Related Questions