svn commit: r211221 - head/usr.sbin/acpi/acpidump

[Bruce Evans]

On Thu, 12 Aug 2010, John Baldwin wrote:

> Dag-Erling Smørgrav wrote:
>> John Baldwin <jhb at FreeBSD.org> writes:
>>> Dag-Erling Smørgrav <des at des.no> writes:

Oops, I replied to the commit mail for the round after this before
noticing this thread.

>>>> Slightly better:
>>>> 
>>>> 	printf("\tClass %u Base Address 0x%jx Length %ju\n\n",
>>>>   	    (unsigned int)tcpa->platform_class, (uintmax_t)paddr, 
>>>> (uintmax_t)len);

Ugh, this has many more style bugs:
- `unsigned int' is spelled u_int in KNF, partly to try to avoid the next
   bug
- `platform_class' has type uint16_t.  u_int is larger than that on all
   supported machines, and also on unsupported ones with 16-31 bit u_ints.
   Thus the cast has almost no effect, and has no effect on the result.
   On unsupported machines with exactly 16-bit u_ints, it has no effect.
   On unsupported and supported machines with >= 17-bit ints, the arg
   gets promoted to a signed int thanks to C90's promotion botch, but
   the value of the signed int is nonnegative so there is no difference
   here from the result of an unbotched promotion to u_int.
- the second line is too ling, and has been nicely mangled by mailers,
   first by putting a hard newline in it and then by quoting it.

>>>> but it would probably be easier to define paddr and len as unsigned long
>>>> long instead of the misspelled u_int64_t, and use %llx and %llu.
>>> Depends.  If the table defines a field to be a 64-bit integer, it is
>>> better to use an explicitly-64-bit integer type such as uint64_t
>>> rather than assuming that 'long long' is 64-bit.

Nah, unsigned long long shouldn't exist.

>> Actually, paddr and len are a memory address and an object size,
>> respectively, so the logical thing would be to use uintptr_t and size_t
>> with uintmax_t casts...

vm_offset_t and vm_size_t would be more logical, but maybe they are too
BSD(Mach vm)-specific for acpi, especially outside of the kernel.

> Except that physical addresses do not always fit in uintptr_t on i386 (think 
> PAE with 36-bit physical addresses and 32-bit uintptr_t, same for the 
> length).  If they truly were a size_t you could use %z without a uintmax_t 
> cast.

Yes, if paddr really means a physical and not a virtual memory address.
PAE has vm_paddr_t for the former.  vm_paddr_t is 32 bits without PAE and
64 bits with PAE.  Printing it and otherwise using it gives the usual
problem that _no_ typedefed type can be printed or otherwise used with a
hard-coded format or type (other than the typedef), but bugs are more
noticable than usual since the type varies widely within a single arch.

Anyway, almost all typedefed types should be cast to [u]intmax_t for
printing, so that you don't have to know too much about what they are.
We are still very sloppy about this for the smaller types, but have
to be more careful for 64-bit types.  Sloppy printing of the smaller
types will break eventually when int or long expands but the typedefs
don't.

Bruce