| Message ID | 20240308133404.2619848-1-patrice.chotard@foss.st.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | fdt_support: fix fdt_copy_fixed_partitions function() | expand |
Hello Patrice, Please, see my comment below. On 2024-03-08 14:34, Patrice Chotard wrote: > Move variable declaration at the beginning of the function. > > Fixes: 163c5f60ebb4 ("fdt_support: add fdt_copy_fixed_partitions > function") > > Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> > --- > > boot/fdt_support.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/boot/fdt_support.c b/boot/fdt_support.c > index 090d82ee80a..f948cf8cd42 100644 > --- a/boot/fdt_support.c > +++ b/boot/fdt_support.c > @@ -1053,9 +1053,10 @@ void fdt_fixup_mtdparts(void *blob, const > struct node_info *node_info, > int fdt_copy_fixed_partitions(void *blob) > { > ofnode node, subnode; > + const u32 *reg; > int off, suboff, res; > char path[256]; > - int address_cells, size_cells; > + int address_cells, size_cells, len; > u8 i, j, child_count; > > node = ofnode_by_compatible(ofnode_null(), "fixed-partitions"); > @@ -1101,9 +1102,6 @@ int fdt_copy_fixed_partitions(void *blob) > if (!ofnode_valid(subnode)) > break; > > - const u32 *reg; > - int len; > - Perhaps it would be better to keep these two variables local to the block they're used in. I mean, in this case it isn't a big deal anyway, but results in a bit cleaner code. > suboff = fdt_find_or_add_subnode(blob, off, > ofnode_get_name(subnode)); > res = fdt_setprop_string(blob, suboff, "label", > ofnode_read_string(subnode, "label"));
On Fri, Mar 08, 2024 at 02:34:04PM +0100, Patrice Chotard wrote: > Move variable declaration at the beginning of the function. > The problem, presumably, is that when declarations are in the middle of a block then it triggers a GCC warning. "declarations after code" or whatever... The commit message is not really clear. And when I built this file I don't get a warning. Is there a specific config required to trigger the warning? Btw, the Linux kernel recently silenced this warning because it doesn't work well with the cleanup.h code... It will be interesting to see if people abandon this style guideline. regards, dan carpenter > Fixes: 163c5f60ebb4 ("fdt_support: add fdt_copy_fixed_partitions function") > > Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> > ---
On 2024-03-11 12:37, Dan Carpenter wrote: > On Fri, Mar 08, 2024 at 02:34:04PM +0100, Patrice Chotard wrote: >> Move variable declaration at the beginning of the function. > > The problem, presumably, is that when declarations are in the middle of > a block then it triggers a GCC warning. "declarations after code" or > whatever... The commit message is not really clear. IIRC, there's at least one more such case of block-local variables in the same source code file, so perhaps we should move those as well, if the final decision is to move some of them. > And when I built this file I don't get a warning. Is there a specific > config required to trigger the warning? > > Btw, the Linux kernel recently silenced this warning because it doesn't > work well with the cleanup.h code... It will be interesting to see if > people abandon this style guideline. > > regards, > dan carpenter > >> Fixes: 163c5f60ebb4 ("fdt_support: add fdt_copy_fixed_partitions >> function") >> >> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> >> ---
On 3/11/24 12:37, Dan Carpenter wrote: > On Fri, Mar 08, 2024 at 02:34:04PM +0100, Patrice Chotard wrote: >> Move variable declaration at the beginning of the function. >> > > The problem, presumably, is that when declarations are in the middle of > a block then it triggers a GCC warning. "declarations after code" or > whatever... The commit message is not really clear. Hi Yes it was my intention. During code review, i noticed this "in the middle" variable declaration and always thought that compiler will warn about this. > > And when I built this file I don't get a warning. Is there a specific > config required to trigger the warning? I confirm, i checked also on my side and don't get any warning. > > Btw, the Linux kernel recently silenced this warning because it doesn't > work well with the cleanup.h code... It will be interesting to see if > people abandon this style guideline. So this patch can be abandoned. Thanks Patrice > > regards, > dan carpenter > >> Fixes: 163c5f60ebb4 ("fdt_support: add fdt_copy_fixed_partitions function") >> >> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> >> --- >
diff --git a/boot/fdt_support.c b/boot/fdt_support.c index 090d82ee80a..f948cf8cd42 100644 --- a/boot/fdt_support.c +++ b/boot/fdt_support.c @@ -1053,9 +1053,10 @@ void fdt_fixup_mtdparts(void *blob, const struct node_info *node_info, int fdt_copy_fixed_partitions(void *blob) { ofnode node, subnode; + const u32 *reg; int off, suboff, res; char path[256]; - int address_cells, size_cells; + int address_cells, size_cells, len; u8 i, j, child_count; node = ofnode_by_compatible(ofnode_null(), "fixed-partitions"); @@ -1101,9 +1102,6 @@ int fdt_copy_fixed_partitions(void *blob) if (!ofnode_valid(subnode)) break; - const u32 *reg; - int len; - suboff = fdt_find_or_add_subnode(blob, off, ofnode_get_name(subnode)); res = fdt_setprop_string(blob, suboff, "label", ofnode_read_string(subnode, "label"));
Move variable declaration at the beginning of the function. Fixes: 163c5f60ebb4 ("fdt_support: add fdt_copy_fixed_partitions function") Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> --- boot/fdt_support.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)