Re: gEDA-user: Removing My* memory alllocation functions

[Stephen Ecob <silicon.on.inspiration@xxxxxxxxx>]

On Thu, Dec 9, 2010 at 12:24 AM, Peter Clifton <pcjc2@xxxxxxxxx> wrote:
> On Wed, 2010-12-08 at 14:26 +1100, Stephen Ecob wrote:
>> Patch0002: we're getting close. I see only one remaining issue:
>>
>> create.c:900: you address this both in patch 0001 and patch 0002
>> In patch 0001 you preserve the behaviour of the existing code
>
> It was still a save MyStrdup -> strdup change, so is valid there.

Yes, I think it is the correct solution.

>> In patch 0002 you unconditionally call strdup()
>
> Was basically what you suggested I think, regarding TextString != NULL.
> I had presumed it was similar to the CreateNewText() case, but now you
> point it out below, I see now why that might not have been correct.

Yes

>> I've looked at the code further, and I think that the approach you
>> take in patch 0001 is correct.
>> Each element has three names: Description, NameOnPCB and Value.  It's
>> entirely possible that at least one of them will be NULL, so an
>> unconditional call would be dangerous.
>
>> Please disregard my comment of 42 minutes ago "create.c:900 Same issue
>> as create.c:593"  It's not the same issue.
>> My $0.02 re create.c:593 still stands, but perhaps a better solution
>> would be to put an assert(TextString != NULL); at the start of
>> CreateNewText()
>
> I'd be ok with that as a separate patch adding the assert. Beware that
> asserts typically aren't switched on in a normal build of PCB though.

Yes, I've enabled assert() once or twice and it wasn't pretty - too
many of the assert()s have suffered code rot.  On my list of things to
address when I can find the time :)

>  TextTypePtr text = GetTextMemory (Layer);
>  if (!text)
>    return (text);
>
> Love to hate that line.. What it means:
>
>  if (text == NULL)
>    return NULL;
>
> It would be tempting to make that
>
>  TextType *text;
>
>  if (TextString == NULL)
>    return NULL;
>
>  text = GetTextMemory (Layer);
>  if (text == NULL)
>    return NULL;
>
>  ....
>
> But it does have the downside of making things fail silently. Perhaps
> the assert is the best way to go.. it adds a note to the programmer of
> what we expect, and the segfault in strdup will still catch any errors
> if asserts are disabled.

I like the code you've written above. If anyone is foolish enough to
call CreateNewText() with NULL text it pushes the problem straight
back at them.  Given that assert()s generally fall on deaf ears, I
think that a patch with this code would be best.


_______________________________________________
geda-user mailing list
geda-user@xxxxxxxxxxxxxx
http://www.seul.org/cgi-bin/mailman/listinfo/geda-user