[U-Boot-Users] [PATCH 16/18] avr32: Fix two warnings in atmel_mci.c

[Haavard Skinnemoen]

<Ken.Fuchs at bench.com> wrote:
> Haavard Skinnemoen wrote:
> 
> > The warnings are harmless but annoying. Let's fix them.
> 
> If the warnings are "harmless", why are you "fixing them"?

They are "harmless" as in "doesn't actually cause any wrong behaviour".
But as Scott points out, they may make warnings that indicate real
problems harder to catch.

For example, if someone comes along and changes the signature of the
"block_read" hook in struct block_dev_desc, I may not notice because
gcc is _already_ warning about a possible mismatch.

> The compiler has switches to turn off warnings, if they
> annoy you too much.

Well, sure. But the "assignment to incompatible pointer type" is
usually very good at catching real API violations, so even if it in
this case catches code that is merely ugly, turning it off altogether
would be the wrong thing to do IMO.

On the other hand, the "may be used uninitialized" warning tends to
come up with a lot of false positives, but there's AFAIK no way to turn
it off without also turning off the "is used uninitialized" warning,
which is very useful.

> Does this refactoring of the code do something more than
> just avoid a warning or two?  If not, I would reject it.

I wouldn't call these tiny changes "refactoring". And I can't see any
downsides except _maybe_ two bytes of additional .text usage.

I have to admit I wasn't sure about this one though:

> > @@ -443,6 +444,7 @@ static void mci_set_data_timeout(struct 
> > mmc_csd *csd)
> >  
> >  	dtocyc = timeout_clks;
> >  	dtomul = 0;
> > +	shift = 0;
> >  	while (dtocyc > 15 && dtomul < 8) {
> >  		dtomul++;
> >  		shift = dtomul_to_shift[dtomul];

While I'm no fan of adding spurious initializations just because gcc
can't figure out what the code is doing well enough to prove that it is
always initialized, I think that in this case, it certainly isn't
obvious, and 0 is a reasonable default.

So I decided to add it anyway, just in case later changes makes the
assumption invalid.

Haavard