Ken.Fuchs@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