From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gdb-patches-return-45048-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Received: (qmail 22741 invoked by alias); 20 Jul 2006 19:32:52 -0000
Received: (qmail 22719 invoked by uid 22791); 20 Jul 2006 19:32:51 -0000
X-Spam-Check-By: sourceware.org
Received: from nitzan.inter.net.il (HELO nitzan.inter.net.il) (192.114.186.20)     by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 20 Jul 2006 19:32:49 +0000
Received: from HOME-C4E4A596F7 (IGLD-83-130-247-14.inter.net.il [83.130.247.14]) 	by nitzan.inter.net.il (MOS 3.7.3-GA) 	with ESMTP id EFM99203 (AUTH halo1); 	Thu, 20 Jul 2006 22:32:42 +0300 (IDT)
Date: Thu, 20 Jul 2006 19:32:00 -0000
Message-Id: <u4pxchy1x.fsf@gnu.org>
From: Eli Zaretskii <eliz@gnu.org>
To: Vladimir Prus <vladimir@codesourcery.com>
CC: gdb-patches@sources.redhat.com
In-reply-to: <200607201341.34070.vladimir@codesourcery.com> (message from 	Vladimir Prus on Thu, 20 Jul 2006 13:41:33 +0400)
Subject: Re: Flash support part 1: memory maps
Reply-to: Eli Zaretskii <eliz@gnu.org>
References:  <200607201341.34070.vladimir@codesourcery.com>
X-IsSubscribed: yes
Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org>
List-Archive: <http://sourceware.org/ml/gdb-patches/>
List-Post: <mailto:gdb-patches@sourceware.org>
List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs>
Sender: gdb-patches-owner@sourceware.org
X-SW-Source: 2006-07/txt/msg00274.txt.bz2

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Thu, 20 Jul 2006 13:41:33 +0400
> 
> this patch is part of my work to add flash memory programming support to gdb. 

Thanks; my comments below.  (Note that I'm not the responsible
maintainer for this are of GDB, though.)

> +/* Parse a field VALSTR that we expect to contain an integer value.
> +   The integer is returned in *VALP.
> +   The string is parsed with the strtoul rountine.
> +
> +   Returns 0 for success, -1 for error.  */
> +static int
> +xml_parse_unsigned_integer (const char *valstr, unsigned long *valp)

Why is this (and other xml_* functions) here?  They seem to be pretty
much unrelated to memory-map.c, and I'd guess that other features that
use XML will want them.  How about a separate file, say gdb-xml.c or
something?

> +  /* Expat interface does not guarantee that a single call to
> +     a handler will be made. Actually, one call for each line
> +     will be made, and character data can possibly span several
> +     lines.
> +
> +     Take care to realloc the data if needed.
> +  */  

This style of comments is not the one prescribed by the GNU coding
standards.

> +  if (!data->character_data)
> +    data->character_data = (char *)malloc (len + 1);
> +  else
> +    {
> +      current_size = strlen (data->character_data);
> +      data->character_data = (char *)realloc (data->character_data,
> +                                              current_size + len + 1);

Why do we need to cast the results of malloc and realloc?