Attention is currently required from: arehbein.

Patch set 6:Code-Review -1

View Change

8 comments:

• File src/vty/tdef_vty.c:

  • Patch Set #6, Line 314: static char *add_group_args(void *talloc_ctx, char *dest, struct osmo_tdef_group *src)

    src can be const.

  • Patch Set #6, Line 327: static char *add_group_docs(void *talloc_ctx, char *dest, struct osmo_tdef_group *src)

    src can be const.

  • Patch Set #6, Line 340: dest = add_group_args(tall_vty_cmd_ctx, dest, src);

    src can be const.

  • Patch Set #6, Line 349: dest = add_group_docs(tall_vty_cmd_ctx, dest, src);

    src can be const.

  • Patch Set #6, Line 358: static void set_show_cmd_strs(struct osmo_tdef_group *template, const char *cmd_group,

    I don't really like the "template" naming, it's not something we have been using so far and imho difficult to gasp. gerrit also seems to agree with it being not a good name as it's marking it as if it was a keyword 😊

    template can be const.

  • Patch Set #6, Line 361: size_t cmd_group_len = cmd_group ? strlen(cmd_group) : 0;

    this is all really hard to read, you are putting to much stuff on any line. Add some spacing and rework it.

    Have a look at osmo_strbuf. Too many strlen() everywhere (hardcoded strings can use sizeof()).

    I'm pretty sure cmd_group_space won't be needed.

  • Patch Set #6, Line 384: static void set_cfg_cmd_strs(struct osmo_tdef_group *template, const char *cmd_group,

    see comments from set_show_cmd_strs().

  • Patch Set #6, Line 445: strcat(memcpy(cmd_group_space, cmd_group, strlen(cmd_group) + 1), " ");

    I think it's first time I see somebody using the return value of memcpy.
    Anyway, all this is too complex, simply use snprintf() or osmo_strbuf.

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