-1

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

closed as off-topic by gnat, David Arno, Bart van Ingen Schenau, Andres F., Greg Burghardt Nov 15 '16 at 13:26

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions asking for assistance in explaining, writing or debugging code are off-topic here. These can be asked on Stack Overflow if they include the desired behavior, a specific problem or error, and the shortest code necessary to reproduce it in the question (see Minimal, Complete, and Verifiable Example)." – David Arno, Bart van Ingen Schenau, Andres F., Greg Burghardt
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 8
    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 3: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 3:23
  • 1
    @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. – immibis Nov 14 '16 at 4:18
  • 2
    Generally, a jump into any clause is an absolute no-go! – Thomas Kilian Nov 14 '16 at 6:33
  • 1
    So, if im_self != Py_None, does Py_DECREF(im_self) execute, or not? Does it leak? – rwong Nov 15 '16 at 10:45
5

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/

2

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.

0

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.

Not the answer you're looking for? Browse other questions tagged or ask your own question.