On Tue, 25 Jan 2022 17:03:03 -0600 Segher Boessenkool segher@kernel.crashing.org wrote:
Hi!
Hi Segher,
Thanks for the feedback. I forgot about this, and came back wondering why this patch wasn't applied.
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.
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.
- 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? 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.
- printf("size=%d, trampoline_size=%d\n",MEMORY_SIZE + (2 *
- printf("size=%ld, trampoline_size=%ld\n",MEMORY_SIZE + (2 * DICTIONARY_SIZE) + TRAMPOLINE_SIZE, TRAMPOLINE_SIZE);
This needs a cast as well.
Etc.
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.
Glenn
Segher