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

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.

I just used a goto statement. Is this OK?

3 Answers3