Quoting Bert Wesarg <[EMAIL PROTECTED]>:
>On Thu, May 15, 2008 at 1:54 AM, Tony Balinski <[EMAIL PROTECTED]> wrote:
>> Quoting Tony Balinski <[EMAIL PROTECTED]>:
>>
>>> Since I find I am unable to add a patch to the sourceforge bug report
>>> mentioned above (am I missing some privileges here?), I'm sending this
>>> patch to the list for perusal.
>>>
>>> Share and enjoy!
>Thanks Tony.
>
>Some comments below:
>
>> +static long numAllocatedSinceLastGC = 0;
>> +static size_t sizeAllocatedSinceLastGC = 0;
>No need to initialize to 0. static vars are automatically 0. But I
>don't know whether this is an Nedit style convention.
True, but I like having explicit initialisers when they make sense (at
least to humans!). There's no cost to the code: it's just an initialised
data segment.
>> +/* structure to link allocated strings */
>> +typedef struct GCStringTag {
>> + struct GCStringTag *next;
>> + char buff[1]; /* buff[0] is the same as the inUse
flag */
>> +} GCString;
>Long overdue and nicely done. These handcoded things where really ugly.
>
>To get more clearity, maybe something like this works too:
>
> union {
> char inUse;
> char buff[1];
> } u;
>
>Or:
>
> #define strInUse buff[1]
>
>and than
>
> str->strInUse
I started with this:
typedef struct GCStringTag {
struct GCStringTag *next;
char inUse;
char buff[1];
} GCString;
but decided not to keep it since I didn't want any problems
(compiler-specific?) with alignment. I wanted this to avoid having to
return &...->buff[1].
I considered the union too, but it wouldn't mean I could avoid the [1]
offset. So I decided to let it drop. As for the #define (which I didn't
think of), it seems a bit overkill for the use made of it and doesn't
follow scoping rules. Maybe function-style #defines, eg
#define GCStringBuff(ptr) (&(ptr)->buff[1])
#define GCStringInUse(ptr) ((ptr)->buff[0])
#define StringInUseGCFlag(str) ((str)[-1])
>> +static void dropContext(RestartData *context)
>> +{
>> + /* unlink it from the list of continuations */
>> + Continuations = context->next;
>> +
>> + context->next->prev = context->prev;
>> + context->prev->next = context->next;
>> +
>> + if (Continuations == context)
>> + Continuations = 0;
>Should be NULL.
Right. That would be better.
>> -/* Allocate a new string buffer of length chars */
>> +/*
>> +** Allocate a new string buffer of length chars
>> +** length does not include terminating null
>> +*/
>This comment is wrong, length actually need to include the terminating
>null.
You're right, it's pretty useless. I wanted to say you have to account for
the null yourself. How about "no extra space is added for a terminating
null here"?
>> +/*
>> +** Marks all strings and/or arrays accessible in all active context stacks
>> +*/
>> +static void garbageCollectMarkActiveData()
>> +{
>> + RestartData sentinel;
>> + RestartData *ctx;
>> +
>> + linkContext(&sentinel);
>Why haven't you uses a sentinal based linked list in the first place?
Force of habit. I can write circular linked lists in my sleep :) Also
aesthetic: I like only having a pointer at the global level, rather than a
"whole" structure for a sentinel. Here I use a temporary sentinel only for
the walk through. Otherwise a do-while could also work:
RestartData *ctx = Continuations;
if (ctx) {
do {
DataValue *dv;
for (dv = ctx->stack; dv != ctx->stackP; ++dv) {
garbageCollectMarkDataValue(dv);
}
ctx = ctx->next;
} while (ctx != Continuations);
}
>I thought some time ago about the semantics of changes to the array
>while iterating it.
>The only sane solution was to unconditionally copy the array for the
>iteration. Some other toughts?
Indeed, you have a point. Copying must be avoided if possible if we want
arrays of decent size to function well. A full reference counted
implementation could manage this with minimal copying. Here we could use an
iterator refcount which would allow us to detect a change being made to a
(non-nested) array that's also the subject of iterations, and take action.
Exactly what action, I haven't thought through yet!
Tony
--
NEdit Develop mailing list - [email protected]
http://www.nedit.org/mailman/listinfo/develop
