[haiku-commits] Re: BRANCH xyzzy-github.x86_64 - data/system/data/fonts/ttfonts
- From: "Ingo Weinhold" <ingo_weinhold@xxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Fri, 22 Jun 2012 00:22:51 +0200
xyzzy-github.x86_64 wrote:
[...]
> ############################################################################
>
> Commit: d8efc6caf6babe278c48e8aa3277376fb68ff455
>
> Author: Alex Smith <alex@xxxxxxxxxxxxxxxx>
> Date: Thu Jun 21 17:02:23 2012 UTC
>
> Changes to kernel_args to make it identical for x86 and x86_64.
>
> * Added a FixedWidthPointer template class which uses 64-bit storage to hold
> a pointer. This is used in place of raw pointers in kernel_args.
> * Added __attribute__((packed)) to kernel_args and all structures contained
> within it. This is necessary due to different alignment behaviour for
> 32-bit and 64-bit compilation with GCC.
Interesting. My experience with compiling for x86-64 has been that packing a
structure often doesn't work since there are alignment constraints that the
compiler doesn't allow to be broken (resulting in a warning). Have you found
any particular examples where it makes a difference? Or is this just about end
of structure padding? If it works too, I'd find forcing structure alignment
(__attribute__((aligned (8)))) preferrable.
[...]
> diff --git a/headers/private/kernel/util/FixedWidthPointer.h
> b/headers/private/kernel/util/FixedWidthPointer.h
> new file mode 100644
> index 0000000..b8ceaaf
> --- /dev/null
> +++ b/headers/private/kernel/util/FixedWidthPointer.h
> @@ -0,0 +1,123 @@
> +/*
> + * Copyright 2012, Alex Smith, alex@xxxxxxxxxxxxxxxxx
> + * Distributed under the terms of the MIT License.
> + */
> +#ifndef KERNEL_UTIL_FIXED_WIDTH_POINTER_H
> +#define KERNEL_UTIL_FIXED_WIDTH_POINTER_H
> +
> +
> +#include <SupportDefs.h>
> +
> +
> +/*!
> + \class FixedWidthPointer
> + \brief Pointer class with fixed size (64-bit) storage.
> +
> + This class is a pointer-like class that uses a fixed size 64-bit storage.
> + This is used to make kernel_args compatible (i.e. the same size) for both
> + 32-bit and 64-bit kernels.
> +*/
> +template<typename Type>
> +class FixedWidthPointer {
> +public:
> + operator Type*() const
> + {
> + return (Type *)(addr_t)fValue;
> + }
> +
> + operator addr_t() const
> + {
> + return (addr_t)fValue;
> + }
> +
> + Type &operator*() const
For new code please prefer "Type& " over "Type &" pointer/reference style
(casts: "(Type*)foo"). Where old code uses the latter consistently just keep it
consistent or change the whole file.
> + {
> + return *(Type *)*this;
> + }
[...]
> + /*!
> + Set the 64-bit pointer value.
> + \param addr New address for the pointer.
> + */
> + void SetTo(uint64 addr)
> + {
> + fValue = addr;
> + }
Blank line here.
> +private:
> + uint64 fValue;
> +} _PACKED;
I'm pretty sure _PACKED doesn't make a difference here. :-)
> +// Specialization for void pointers, can be converted to another pointer
> type.
> +template<>
> +class FixedWidthPointer<void> {
I wonder, wouldn't have a templatized cast operator in the generic version the
same effect and work more generally? Like:
template<typename TargetType>
operator TargetType*() const
{
return static_cast<TargetType*>((Type*)(addr_t)fValue);
}
I haven't tested it, so it might not work. Alternatively (maybe even
preferrably) the caller can simply cast twice ((Foo*)(Bar*)pointerObject). Or a
templatized cast method (CastAs()) could be added.
[...]
> diff --git a/src/system/boot/platform/bios_ia32/debug.cpp
> b/src/system/boot/platform/bios_ia32/debug.cpp
> index 73d2906..deb389d 100644
> --- a/src/system/boot/platform/bios_ia32/debug.cpp
> +++ b/src/system/boot/platform/bios_ia32/debug.cpp
> @@ -162,7 +162,7 @@ debug_cleanup(void)
>
> if (!gKernelArgs.keep_debug_output_buffer) {
> gKernelArgs.debug_output = kernel_args_malloc(sBufferPosition);
> - if (gKernelArgs.debug_output != NULL) {
> + if (gKernelArgs.debug_output) {
This is not so nice. It should be possible to support this syntax. I believe
adding ==/!=(const Type *) operators does the trick.
[...]
> diff --git a/src/system/kernel/fs/vfs_boot.cpp
> b/src/system/kernel/fs/vfs_boot.cpp
> index b8240d7..c01a132 100644
> --- a/src/system/kernel/fs/vfs_boot.cpp
> +++ b/src/system/kernel/fs/vfs_boot.cpp
> @@ -54,8 +54,6 @@ static struct {
> {NULL}
> };
>
> -static int32 sBootMethodType;
> -
> // This can be used by other code to see if there is a boot file system
> already
> dev_t gBootDevice = -1;
> bool gReadOnlyBootDevice = false;
> @@ -322,30 +320,27 @@ DiskBootMethod::SortPartitions(KPartition** partitions,
> int32 count)
> The boot code should then just try them one by one.
> */
> static status_t
> -get_boot_partitions(kernel_args* args, PartitionStack& partitions)
> +get_boot_partitions(KMessage &bootVolume, PartitionStack& partitions)
Wrong pointer style.
[...]
> diff --git a/src/system/boot/platform/cfe/arch/ppc/mmu.cpp
> b/src/system/boot/platform/cfe/arch/ppc/mmu.cpp
> index 2f9b2a0..b20495e 100644
> --- a/src/system/boot/platform/cfe/arch/ppc/mmu.cpp
> +++ b/src/system/boot/platform/cfe/arch/ppc/mmu.cpp
[...]
> @@ -262,8 +261,9 @@ find_free_physical_range(size_t size)
> }
>
> for (uint32 i = 0; i < gKernelArgs.num_physical_allocated_ranges; i++) {
> - void *address = (void *)(gKernelArgs.physical_allocated_range[i].start
> - + gKernelArgs.physical_allocated_range[i].size);
> + void *address =
> + (void *)(addr_t)(gKernelArgs.physical_allocated_range[i].start
> + + gKernelArgs.physical_allocated_range[i].size);
Please always break the line before the operator. The only exception is the
comma operator, which should be used like the comma in argument lists. A line
with a ternary operator can be broken before either (or both) operator symbols.
[...]
> diff --git a/src/system/boot/platform/openfirmware/arch/ppc/mmu.cpp
> b/src/system/boot/platform/openfirmware/arch/ppc/mmu.cpp
> index bbe1126..7f46272 100644
> --- a/src/system/boot/platform/openfirmware/arch/ppc/mmu.cpp
> +++ b/src/system/boot/platform/openfirmware/arch/ppc/mmu.cpp
[...]
> @@ -443,8 +442,9 @@ find_free_physical_range(size_t size)
> }
>
> for (uint32 i = 0; i < gKernelArgs.num_physical_allocated_ranges; i++) {
> - void *address = (void *)(gKernelArgs.physical_allocated_range[i].start
> - + gKernelArgs.physical_allocated_range[i].size);
> + void *address =
> + (void *)(addr_t)(gKernelArgs.physical_allocated_range[i].start
> + + gKernelArgs.physical_allocated_range[i].size);
Ditto.
CU, Ingo
Other related posts:
