I just used a goto statement. Is this OK?

Question

I just wrote this code that uses a goto statement.

if (PyMethod_Check(attrib_value)) {
    PyObject *im_self = PyObject_GetAttrString(attrib_value, "im_self");
    if (im_self == Py_None) {
        js_value = FunctionTemplate::New(isolate, py_class_method_callback, js_name, signature);
    } else {
        goto method_counts_as_function;
    }
    Py_DECREF(im_self);
} else if (PyFunction_Check(attrib_value)) {
method_counts_as_function:
    js_value = ((py_function *) py_function_to_template(attrib_value))->js_template->Get(isolate);
    templ->Set(js_name, js_value);
} else {
    js_value = js_from_py(attrib_value, no_ctx);
    templ->Set(js_name, js_value);
}

The intention is to treat certain methods like functions. Normally I would use a && in the if statement, something like this:

if (PyMethod_Check(attrib_value) &&
    PyObject_GetAttrString(attrib_value, "im_self) != Py_None)

Unfortunately, PyObject_GetAttrString returns an object with an incremented reference count. I can't decrement the reference count on a value that may or may not have existed in the second part of the short-circuit and.

I feel pretty bad about this since goto statements are considered harmful, but I can't think of a good way to rewrite this without gotos. Is this OK?

Code in context

The `goto` is a symptom that the code is too convoluted. Which it certainly is in this case. Seriously, you should take a break and come back to this code is a week or so, and see if you have any memory of what's going on. Will be really eye opening. — user949300, Nov 14 '16 at 03:05
It does match one of the patterns I've seen used to justify gotos: if (condition 1) { code; if (condition 2) { more code; } else { goto else; } } else { else: code; } — tbodt, Nov 14 '16 at 03:23
What is "a variable that may or may not have existed"? In C, variables either exist, or they don't. — user253751, Nov 14 '16 at 03:38
@immibis I mean a value that I never assigned to a variable in the second part of a short-circuit or. If the first part is true, the value never existed. — tbodt, Nov 14 '16 at 03:40
@immibis Probably the best idea. I'm slightly worried that it would move important code about 20-30 lines away, but that's probably because the function it's in needs to be broken up. (It's 77 lines.) — tbodt, Nov 14 '16 at 04:11
@tbodt Functions don't need to be broken up if they're a certain number of lines, and ignore anyone who says otherwise. They need to be broken up if doing so makes the code better (more understandable, less duplication, etc). In this case I think it would make the code more understandable. — user253751, Nov 14 '16 at 04:18
@immibis My reason for breaking up this function would be to make it less dense. Turns out there's an existing function that lets me check whether a method is bound without worrying about reference counts, so that's probably the best solution here. — tbodt, Nov 14 '16 at 04:21
Thomas, I have NO idea what the code that jumps into an if clause will do, and I bet 50% of all programmers would guess wrong. — user949300, Nov 15 '16 at 01:24
At a minimum, I'd expect a clear explanation of why it is needed in the surrounding comments. — RemcoGerlich, Nov 15 '16 at 10:40
So, if `im_self != Py_None`, does `Py_DECREF(im_self)` execute, or not? ***Does it leak?*** — rwong, Nov 15 '16 at 10:45
"because goto is considered harmful" - no, goto isn't considered harmful. Dijkstra (I think) once wrote a paper with "goto considered harmful" as the click-bait title, that's all. That said, a goto jumping into an if statement inside an else statement, that's evil. — gnasher729, Nov 15 '16 at 22:32
@tbodt In fact, I want to point out that the badness of `goto` is that careless use may cause important "must-execute" statements to be skipped over. Therefore, when using goto, trace every possible path of execution carefully, and perhaps put that in the code comments. Btw, some languages such as C++, C# have some support for scope-based destruction, and could have prevented this logic error. — rwong, Nov 15 '16 at 23:34

score 5 · Answer 1 · answered Nov 14 '16 at 04:08

I can't think of a good way to rewrite this without gotos.

I guess that this:

if (PyMethod_Check(attrib_value)) {
    PyObject *im_self = PyObject_GetAttrString(attrib_value, "im_self");
    if (im_self == Py_None) {
        js_value = FunctionTemplate::New(isolate, py_class_method_callback, js_name, signature);
    } else {
        goto method_counts_as_function;
    }
    Py_DECREF(im_self);
} else if (PyFunction_Check(attrib_value)) {
method_counts_as_function:
    js_value = ((py_function *) py_function_to_template(attrib_value))->js_template->Get(isolate);
    templ->Set(js_name, js_value);
} else {
    js_value = js_from_py(attrib_value, no_ctx);
    templ->Set(js_name, js_value);
}

... can be refactored as follows. Put the first bit in a subroutine which returns true or false depending on whether you want to do the special thing:

SUBROUTINE/FUNCTION:

if (PyMethod_Check(attrib_value)) {
    PyObject *im_self = PyObject_GetAttrString(attrib_value, "im_self");
    if (im_self == Py_None) {
        js_value = FunctionTemplate::New(isolate, py_class_method_callback, js_name, signature);
    } else {
        RETURN TRUE
    }
    Py_DECREF(im_self);

    RETURN FALSE

} else if (PyFunction_Check(attrib_value)) {

RETURN TRUE

}

... and ...

if (SUBROUTINE/FUNCTION){

    /// method_counts_as_function:
    js_value = ((py_function *) py_function_to_template(attrib_value))->js_template->Get(isolate);
    templ->Set(js_name, js_value);
} else {
    js_value = js_from_py(attrib_value, no_ctx);
    templ->Set(js_name, js_value);
}

It would be more complicated if you also had a return a local variable like im_self (in order to use it subsequently), but in this case I think you don't.

Is this OK?

https://xkcd.com/292/

Doc Brown · Answer 2 · 2016-11-16T09:57:32.180

Put this line into a function (I guess you can come up with a better name):

py_function *get_js_value(attrib_value_t attrib_value,isolate_t isolate)
{
     return ((py_function *) py_function_to_template(attrib_value))
                             ->js_template->Get(isolate);
}

Then you can rewrite the code as follows:

if (PyMethod_Check(attrib_value)) {
    PyObject *im_self = PyObject_GetAttrString(attrib_value, "im_self");
    if (im_self == Py_None) {
        js_value = FunctionTemplate::New(isolate, py_class_method_callback, js_name, signature);
    } else {
        js_value =get_js_value(attrib_value,isolate);
    }
    Py_DECREF(im_self);
} else if (PyFunction_Check(attrib_value)) { 
  {
      js_value =get_js_value(attrib_value,isolate);
  } else {
      js_value = js_from_py(attrib_value, no_ctx);
  }
  templ->Set(js_name, js_value);
}

(Note this also removes the duplication of the templ->Set( part.)

This replaces the goto by a repeated call to the same function, and it leaves the code in a state with almost no duplicated logic.

score 1 · Answer 3 · 2016-11-14T04:49:07.970

My suggestion is to "duplicate the code" and let the compiler do "Common Sub-expression elimination" (CSE) for you. Smart compilers should be able to find the common part of your code and generate branches accordingly:

if (PyMethod_Check(attrib_value)) {
  PyObject *im_self = PyObject_GetAttrString(attrib_value, "im_self");
  if (im_self == Py_None) {
    js_value = FunctionTemplate::New(isolate, py_class_method_callback, js_name, signature);
    Py_DECREF(im_self); // <<<< moved here
  } else {
    //goto method_counts_as_function;
    js_value = ((py_function *) py_function_to_template(attrib_value))->js_template->Get(isolate);
    templ->Set(js_name, js_value);
  }
  //Py_DECREF(im_self); // moved up >>>>
} else if (PyFunction_Check(attrib_value)) {
//method_counts_as_function:
  js_value = ((py_function *) py_function_to_template(attrib_value))->js_template->Get(isolate);
  templ->Set(js_name, js_value);
} else {
  js_value = js_from_py(attrib_value, no_ctx);
  templ->Set(js_name, js_value);
}

If compiler fail to do so or you don't like duplicate codes, you can still define a short macro or an inline function for the duplicated part, and call it in both places.

+1 because that is what I would have done. It's a trivial change,so I can't figure out how OP found this hard to figure out. I don't consider goto harmful; I consider goto that goes into another block harmful. — gnasher729, Sep 05 '19 at 21:23

I just used a goto statement. Is this OK?

3 Answers3