Attention is currently required from: jolly, pespin.

View Change

5 comments:

• File s1gw/S1GW_ConnHdlr.ttcn:

  • Patch Set #2, Line 1210: in ERabIdxList erabs_release := {})

    I vote for stopping adding more "in" churn :D

    For `ERabIdxList`, yes, we don't really need `in/out`. However we should keep using them for `ERabList`, to let the caller know if the given list is going to be modified or not.

  • Patch Set #2, Line 1720: in ERabIdxList erabs_forward,

    No more "in" churn please.

    Why are you so obsessed with this?

  • Patch Set #2, Line 1718:

    function f_ConnHdlr_handover_cmd(MME_UE_S1AP_ID mme_ue_id,
    				 ENB_UE_S1AP_ID enb_ue_id,
    				 in ERabIdxList erabs_forward,
    		
    

    Nitpick: in the existing API, arguments `mme_ue_id` and `enb_ue_id` usually follow `ERabList`/`ERabIdxList` arguments. For the sake of consistency, I suggest:

    ```suggestion
    function f_ConnHdlr_handover_cmd(ERabIdxList erabs_forward,
                                     ERabIdxList erabs_release,
                                     MME_UE_S1AP_ID mme_ue_id,
    				 ENB_UE_S1AP_ID enb_ue_id)
    ```

• File s1gw/S1GW_Tests.ttcn:

  • Patch Set #2, Line 911: for (var integer i := 0; i < lengthof(erabs_forward); i := i + 1) {

    I see this logic repeated in several places, so I think it's worth having a function in `S1GW_ConnHdlr` (next to `f_ERabList_find_by_pdu()`):

    ```
    function f_ERabList_compose(ERabIdxList idx_list)
    runs on ConnHdlr return ERabList {
        var ERabList erabs;

        for (var integer i := 0; i < lengthof(idx_list); i := i + 1) {
            var ERabIdx idx := idx_list[i];
            erabs[i] := g_pars.erabs[idx];
        }
        
        return erabs;
    }
    ```

    This way here you could do:

    ```
    if (lengthof(erabs_forward) > 0) {
        var ERabList erabs := f_ERabList_compose(erabs_forward);
        f_ConnHdlr_erab_release_cmd(erabs, g_pars.mme_ue_id, g_pars.idx);
        f_ConnHdlr_erab_release_rsp(erabs, g_pars.mme_ue_id, g_pars.idx);
    }
    ```

  • Patch Set #2, Line 912:

    tabs vs spaces

To view, visit change 41094. To unsubscribe, or for help writing mail filters, visit settings.