On Fri, 28 Jul 2023 15:08:59 -0500 Segher Boessenkool segher@kernel.crashing.org wrote:
Hi!
On Fri, Jul 28, 2023 at 01:28:20PM -0500, Glenn Washburn wrote:
On Tue, 25 Jan 2022 17:03:03 -0600 Segher Boessenkool segher@kernel.crashing.org wrote:
On Tue, Jan 25, 2022 at 04:27:50PM -0600, Glenn Washburn wrote:
- printk("dict1 %lx dict2 %lx dict %lx\n",dict_one, dict_two, dict);
- printk("dict1 %p dict2 %p dict %p\n",dict_one, dict_two, dict);
This is not identical output (it prefixes 0x to non-0). The better solution (it is needed in many other cases!) is to cast the argument:
printk("dict1 %lx dict2 %lx dict %lx\n", (long)dict_one, (long)dict_two, (long)dict);
Yes, not identical, but does it matter here? This is a debug print statement, not something that a program should be relying on the output format. These are pointers, so the %p format seems, if anything, more correct.
It may not matter, sure. But your patch message should then say it changed this output. Silent changes are bad.
printk("reloc %d %lx\n",i+1, reloc_table[i]);
printk("reloc %d "FMT_ucellx"\n",i+1, reloc_table[i]);Same here.
I'm not seeing why this is the same as above. The FMT_ucellx macro's purpose is to spit out the correct format code for printing a ucell type in the hex display variant. So if typeof(reloc_table[i]) is a ucell, then it should be used.
It is so much easier to read (as well as write!) to just do a cast where you pass the arg :-)
I'm I wrong in understanding that you're against the concept of FMT_ucellx? It seems like there's no need for it if you're casting. One of the purposes, is to abstract away type size differences on different architectures. But from your point of view, wouldn't you just cast to the largest size and use the format code for that size?
Glenn
- printk("building dictionary, %d primitives.\nbuilt words:",
- printk("building dictionary, %lu primitives.\nbuilt words:", sizeof(wordnames) / sizeof(void *));
This is incorrect: sizeof produces a size_t, not a long. So:
printk("building dictionary, %lu primitives.\nbuilt words:", (long)(sizeof wordnames / sizeof wordnames[0]));
Should be cast to (unsigned long), right?
Either is correct here, as required by the C standard.
Of course, if the sizeof is returning something large enough to make a difference, then there are probably other issues, but still for correctness sake.
No, "long" and "unsigned long" can be used interchangeably in varargs. You are correct that it may be easier to understand the longer form :-)
All stdarg functions always need explicit casts (if the argument type isn't correct already). It is a good idea to always do it if the arg isn't obviously a compatible type (or a signed or unsigned variant of it). That way you don't have to think so much, and any reader of your code doesn't either! :-)
Point taken. I like to avoid casting by using the appropriate format code, but I don't think that invalidates the soundness of the above.
Yeah, if and when the actual type passed is clear, just write that in the conversion specifier. My point is that very often it is way easier and way less error-prone to just do a cast!
Segher