Re: [Openocd-development] [PATCH]style: typedefs are evil
On Tue, Aug 30, 2011 at 6:47 PM, Andreas Fritiofson andreas.fritiof...@gmail.com wrote: On Tue, Aug 30, 2011 at 9:25 PM, Eric Wetzel thewet...@gmail.com wrote: On the discussion of style, I mentioned that the Linux coding standard doesn't really like typedefs. Here is an article from Greg K-H that partially addresses the subject: http://www.linuxjournal.com/article/5780?page=0,2 Regarding the patches, the ELF types in replacements.h (and their usage in image.c) should be left as is, because they are replacements for a system header. Here are some new patch files. They are the same changes I already made, but broken up slightly more at Øyvind's request. No more than 50 lines changed per file, and I split the Elf32 header changes out into the 4th patch file, so it can be ignored if desired. It seems to me like Linux is breaking its own rules with that one: typedefs hiding simple, fixed-width types and typedefs hiding the fact that Elf32_Ehdr/Phdr are structures. ~Eric 0001-style-typedefs-are-evil.patch Description: Binary data 0002-style-typedefs-are-evil.patch Description: Binary data 0003-style-typedefs-are-evil.patch Description: Binary data 0004-style-typedefs-are-evil.patch Description: Binary data ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH]style: typedefs are evil
Hi,
just removed all typedefs in the ULINK driver. Patch #1 removes the
typedefs of the USB descriptor structures in the ULINK firmware usb.c
module, patch #2 removes shorttypes.h and changes all of the types that
were defined in that file to types from stdint.h.
On Wed, 2011-08-31 at 08:02 -0400, Eric Wetzel wrote:
Here are some new patch files. They are the same changes I already
made, but broken up slightly more at Øyvind's request. No more than 50
lines changed per file, and I split the Elf32 header changes out into
the 4th patch file, so it can be ignored if desired. It seems to me
like Linux is breaking its own rules with that one: typedefs hiding
simple, fixed-width types and typedefs hiding the fact that
Elf32_Ehdr/Phdr are structures.
I could not get your patch #2 to apply to the current HEAD (fails at the
ulink.c part) because the patches I sent a few days ago were merged
today, so I re-made the changes from this patch to remove the typedefs
from the ULINK driver (ulink.c). This is my patch #3.
Changes are tested with ULINK adapter + Olimex STM32-P103 eval board.
Best regards,
Martin
From 1cc841992b4ef697a94d7d05eb2bc8bfc9073902 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Schm=C3=B6lzer?=
martin.schmoel...@student.tuwien.ac.at
Date: Wed, 31 Aug 2011 00:20:35 +0200
Subject: [PATCH 1/3] ULINK driver: Remove typedefs in OpenULINK firmware USB
descriptor structures
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Martin Schmölzer martin.schmoel...@student.tuwien.ac.at
---
src/jtag/drivers/OpenULINK/include/usb.h | 34 ++--
src/jtag/drivers/OpenULINK/src/usb.c | 48 +
2 files changed, 45 insertions(+), 37 deletions(-)
diff --git a/src/jtag/drivers/OpenULINK/include/usb.h b/src/jtag/drivers/OpenULINK/include/usb.h
index f70819e..a507c9e 100644
--- a/src/jtag/drivers/OpenULINK/include/usb.h
+++ b/src/jtag/drivers/OpenULINK/include/usb.h
@@ -48,7 +48,7 @@
#define STR_DESCR(len,...) { len*2+2, DESCRIPTOR_TYPE_STRING, { __VA_ARGS__ } }
/** USB Device Descriptor. See USB 1.1 spec, pp. 196 - 198 */
-typedef struct {
+struct usb_device_descriptor {
u8 bLength; /// Size of this descriptor in bytes.
u8 bDescriptorType; /// DEVICE Descriptor Type.
u16 bcdUSB; /// USB specification release number (BCD).
@@ -63,10 +63,10 @@ typedef struct {
u8 iProduct;/// Index of product string descriptor.
u8 iSerialNumber; /// Index of string descriptor containing serial #.
u8 bNumConfigurations; /// Number of possible configurations.
-} usb_device_descriptor_t;
+};
/** USB Configuration Descriptor. See USB 1.1 spec, pp. 199 - 200 */
-typedef struct {
+struct usb_config_descriptor {
u8 bLength; /// Size of this descriptor in bytes.
u8 bDescriptorType; /// CONFIGURATION descriptor type.
u16 wTotalLength;/// Combined total length of all descriptors.
@@ -75,10 +75,10 @@ typedef struct {
u8 iConfiguration; /// Index of configuration string descriptor.
u8 bmAttributes;/// Configuration characteristics.
u8 MaxPower;/// Maximum power consumption in 2 mA units.
-} usb_config_descriptor_t;
+};
/** USB Interface Descriptor. See USB 1.1 spec, pp. 201 - 203 */
-typedef struct {
+struct usb_interface_descriptor {
u8 bLength; /// Size of this descriptor in bytes.
u8 bDescriptorType; /// INTERFACE descriptor type.
u8 bInterfaceNumber;/// Interface number.
@@ -88,48 +88,48 @@ typedef struct {
u8 bInterfaceSubclass; /// Subclass code.
u8 bInterfaceProtocol; /// Protocol code.
u8 iInterface; /// Index of interface string descriptor.
-} usb_interface_descriptor_t;
+};
/** USB Endpoint Descriptor. See USB 1.1 spec, pp. 203 - 204 */
-typedef struct {
+struct usb_endpoint_descriptor {
u8 bLength; /// Size of this descriptor in bytes.
u8 bDescriptorType; /// ENDPOINT descriptor type.
u8 bEndpointAddress;/// Endpoint Address: USB 1.1 spec, table 9-10.
u8 bmAttributes;/// Endpoint Attributes: USB 1.1 spec, table 9-10.
u16 wMaxPacketSize; /// Maximum packet size for this endpoint.
u8 bInterval; /// Polling interval (in ms) for this endpoint.
-} usb_endpoint_descriptor_t;
+};
/** USB Language Descriptor. See USB 1.1 spec, pp. 204 - 205 */
-typedef struct {
+struct usb_language_descriptor {
u8 bLength; /// Size of this descriptor in bytes.
u8 bDescriptorType; /// STRING descriptor type.
u16 wLANGID[]; /// LANGID codes.
-} usb_language_descriptor_t;
+};
/** USB String Descriptor. See USB 1.1 spec, pp. 204 - 205 */
-typedef struct {
+struct usb_string_descriptor {
u8 bLength; /// Size of this descriptor in bytes.
u8 bDescriptorType; /// STRING descriptor type.
u16 bString[];
Re: [Openocd-development] [PATCH]style: typedefs are evil
Merged. Thanks! -- Øyvind Harboe - Can Zylin Consulting help on your project? US toll free 1-866-980-3434 / International +47 51 87 40 27 http://www.zylin.com/ ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH]style: typedefs are evil
I think this is the right direction, but I'd like to see the patches split up and verified that there is more than just some silly renaming going on... -- Øyvind Harboe - Can Zylin Consulting help on your project? US toll free 1-866-980-3434 / International +47 51 87 40 27 http://www.zylin.com/ ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH]style: typedefs are evil
Hi, On Tue, 2011-08-30 at 15:25 -0400, Eric Wetzel wrote: On the discussion of style, I mentioned that the Linux coding standard doesn't really like typedefs. Here is an article from Greg K-H that partially addresses the subject: http://www.linuxjournal.com/article/5780?page=0,2 I don't really like typedefs either I just read through this article and agree. I don't think I have a way to verify the changes I made to src/flash/ocl/at91sam7x (from the second patch) or the changes I would make to src/jtag/drivers/OpenULINK. I'm the author of the ULINK driver. From a first glance, I think it is possible to fully get rid of all the typedefs in this driver and firmware. Tomorrow I'll take a closer look at the changes and report back. Best regards, Martin Schmölzer ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH]style: typedefs are evil
On Wed, Aug 31, 2011 at 12:27 AM, Martin Schmölzer martin.schmoel...@student.tuwien.ac.at wrote: Hi, On Tue, 2011-08-30 at 15:25 -0400, Eric Wetzel wrote: I don't think I have a way to verify the changes I made to src/flash/ocl/at91sam7x (from the second patch) or the changes I would make to src/jtag/drivers/OpenULINK. I'm the author of the ULINK driver. From a first glance, I think it is possible to fully get rid of all the typedefs in this driver and firmware. Tomorrow I'll take a closer look at the changes and report back. Just checked and the motivation for using shorter types (easier to stay within 80 character maximum line length, from shorttypes.h) is unfounded. Only one or two lines will grow beyond 80 chars because of the longer names, and other lines are far longer anyway. /Andreas ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH]style: typedefs are evil
On Tue, Aug 30, 2011 at 9:25 PM, Eric Wetzel thewet...@gmail.com wrote: On the discussion of style, I mentioned that the Linux coding standard doesn't really like typedefs. Here is an article from Greg K-H that partially addresses the subject: http://www.linuxjournal.com/article/5780?page=0,2 I don't really like typedefs either, so I started writing a script called 'detypedefifier' that will remove typedefs. So far, it only finds them, but I found and removed a good portion of the typedefs. The ones that remain are either function pointer typedefs, have far-reaching consequences, or are generally in places I don't really want to touch right now. Here is a list of the non-function pointer typedefs that remain after my patches: src/flash/mflash.h: Simple typedef: mg_io_uint32 = unsigned long Simple typedef: mg_io_uint16 = unsigned short Simple typedef: mg_io_uint8 = unsigned char These are probably supposed to be roll-your-own fixed-width types. If the mflash code depends on these actually being fixed-width, it is broken as it is. Unless someone who knows the mflash code objects, these should be replaced with standard uintX_t types. src/helper/types.h: Simple typedef: _Bool = int Simple typedef: _Bool = bool Simple typedef: intptr_t = CYG_ADDRWORD Simple typedef: intmax_t = int64_t Simple typedef: uintmax_t = uint64_t These are compatibility typedefs, don't touch them. src/jtag/drivers/OpenULINK/include/shorttypes.h: Simple typedef: s8 = int8_t Simple typedef: s16 = int16_t Simple typedef: s32 = int32_t Simple typedef: u8 = uint8_t Simple typedef: u16 = uint16_t Simple typedef: u32 = uint32_t Ok, the standard types are unnecessarily long, but they're standard. I always disliked the idea of introducing new typedefs even if it saves 4 or 5 characters per declaration. Wouldn't mind getting rid of these even though there's an exception for them in the kernel style guide. src/jtag/jtag.h: Simple typedef: jtag_callback_data_t = intptr_t src/rtos/rtos.h: Simple typedef: threadid_t = int64_t Simple typedef: symbol_address_t = int64_t Possibly these falls under the one of the kernel style guide exceptions, but I haven't checked their usage. Regarding the patches, the ELF types in replacements.h (and their usage in image.c) should be left as is, because they are replacements for a system header. /Andreas ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
