On Wed, Sep 16, 2015 at 3:21 PM, Nick Stevens <nick.stev...@digi.com> wrote:
>> > +
>> > + if (g_regex_match_full (r, reply, strlen (reply), 0, 0, &match_info,
>> > &match_error)) {
>>
>> I think you should also just restructure the function with a 'goto' for
>> error handling to break out early. Yeah it's a goto, but it's OK in
>> patterns like this for error handling.
>>
>> Also, I'd have local variables for the 'status' and 'pdu' bits so that
>> we don't allocate the returned 'info' until we know it's going to be
>> valid, that way we don't have to free it in the error cases at all.
>> It's usually best not to look too much at 'error'; eg a function
>> shouldn't care whether the caller passed in NULL or a valid pointer to
>> an error, but the patch has "if (*error) info = NULL;". We can avoid
>> that by using the goto stuff.
>
> That's kind of funny, because I actually had structured v1 of the patch with
> gotos. When I moved the code to mm-modem-helpers, I noticed that many of the
> parse functions seemed to avoid goto (mm-3gpp_parse_cgdcont_read_response as
> an arbitrary example), so I was trying to match the style of that module. I'm
> definitely okay switching to gotos for this patch, so long as we're okay with
> having the different style. Thoughts?
I'm fine with gotos for the case dcbw has stated; i.e. for clean error
handling. Just make sure all the possible execution paths get
correctly handled :)
--
Aleksander
https://aleksander.es
_______________________________________________
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
