Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zebra: fix compiler warning about truncation. #15248

Merged
merged 1 commit into from
Jan 29, 2024
Merged

zebra: fix compiler warning about truncation. #15248

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions zebra/zebra_cli.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,12 @@ DEFUN_YANG_NOSH (link_params,

ret = nb_cli_apply_changes(vty, NULL);
if (ret == CMD_SUCCESS) {
char xpath[XPATH_MAXLEN];
char *xpath;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're going to touch this now, can we move the declaration to the beginning of the function?

Copy link
Contributor Author

@choppsv1 choppsv1 Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this part of our style guide?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW in the past the main justification for the preference for all variables to be declared at the beginning of the function was to track stack usage by the function.

This was of particular interest inside kernels where you may be working with a limited per-proces kernel stack. But, we aren't a kernel and this isn't even some large array of data (which still might warrant being at the top of the function for same reasons). Rather it's a temporary variable with recognizable limited scope so I actually think it's in the right place as is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought that this was a common FRR practice/convention, so I've often applied it in reviews (and my own FRR code).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe should say a little more there: I like that the convention helps avoid shadowing problems, and helps avoid endless nitpicking about where exactly declarations should appear (once the box is opened).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chris just keeps the location as-is in this commit, and it's fixing a build failure on newer compiler versions that some people already reported. So I'd like to merge it now and not wait for another CI cycle.

I personally like the current version, because it defines a value in a limited scope, uses it immediately and removes. I think it makes more sense than defining it at the top. Anyway, we can decide on the general convention separately and fix this and other similar definitions separately if needed.


snprintf(xpath, sizeof(xpath), "%s/frr-zebra:zebra/link-params",
VTY_CURR_XPATH);
xpath = asprintfrr(MTYPE_TMP, "%s/frr-zebra:zebra/link-params",
VTY_CURR_XPATH);
VTY_PUSH_XPATH(LINK_PARAMS_NODE, xpath);
XFREE(MTYPE_TMP, xpath);
}

return ret;
Expand Down
Loading