From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25969 invoked by alias); 15 Jul 2014 13:33:19 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 25959 invoked by uid 89); 15 Jul 2014 13:33:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Tue, 15 Jul 2014 13:33:17 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 5B5E111624D; Tue, 15 Jul 2014 09:33:15 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id fjL52MZuWojC; Tue, 15 Jul 2014 09:33:15 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 2B7F41161CA; Tue, 15 Jul 2014 09:33:15 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id AF4C340EBF; Tue, 15 Jul 2014 06:33:16 -0700 (PDT) Date: Tue, 15 Jul 2014 14:00:00 -0000 From: Joel Brobecker To: Pierre Langlois Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Add support for the __flash qualifier on AVR Message-ID: <20140715133316.GE4888@adacore.com> References: <1404816844-1639-1-git-send-email-pierre.langlois@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1404816844-1639-1-git-send-email-pierre.langlois@embecosm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-07/txt/msg00376.txt.bz2 Hello, > 2014-07-08 Pierre Langlois > > gdb/ > * avr-tdep.c (AVR_TYPE_ADDRESS_CLASS_FLASH): New macro. > (AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH): Likewise. > (avr_address_to_pointer): Check for AVR_TYPE_ADDRESS_CLASS_FLASH. > (avr_pointer_to_address): Likewise. > (avr_address_class_type_flags): New function. > (avr_address_class_type_flags_to_name): Likewise. > (avr_address_class_name_to_type_flags): Likewise. > (avr_gdbarch_init): Set address_class_type_flags, > address_class_type_flags_to_name and > address_class_name_to_type_flags. > > gdb/testsuite/ > * gdb.arch/avr-flash-qualifer.c: New. > * gdb.arch/avr-flash-qualifer.exp: New. Sorry about the delay in reviewing this. Some comments below. > +/* Map DW_AT_address_class attributes to a type_instance_flag_value. Note that > + this attribute is only valid with pointer types and therefore the flag is set > + to the pointer type and not its target type. */ Can you start the comment by saying that this function implements the address_class_name_to_type_flags gdbarch method? You can then add the comment above as a second paragraph, or perhaps as an implementation comment inside the function body itself. > +static int > +avr_address_class_type_flags (int byte_size, int dwarf2_addr_class) > +{ Please add a comment saying that this function implements the address_class_type_flags gdbarch method. Generally speaking, all new functions without exception should have an introductory comment. > + // __flash qualifier Please do not use C++-style comments for the moment. > + if (dwarf2_addr_class == 1 && byte_size == 2) > + return AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH; > + return 0; > +} > + > +/* Convert a type_instance_flag_value to an address space qualifier and > + vice-versa. */ > + > +static const char* > +avr_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags) Same as above regarding the function's introductory comment. And also, I'd prefer it if we avoided two functions being documented with the same comment, as this opens the door for a function becoming undocumented if the two functions somehow get separated. > +{ > + if (type_flags & AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH) > + return "flash"; > + else > + return NULL; > +} > + > +static int > +avr_address_class_name_to_type_flags (struct gdbarch *gdbarch, > + const char* name, > + int *type_flags_ptr) Missing introductory comment ("Implements the ..."). -- Joel