Hi,Vojta
thanks for comment

> If you realize that you forget to add some files to Bazaar and you
> haven't pushed the branch, it is better to uncommit the last change
> and add the files. It is better if every revision is at least
> compilable.
thanks.

> I wonder why you decided to limit number of undo operations.
I just want to simplify the implementation.
The implementation is not efficient in memory, 
and I'm afraid it will use up memory quickly.

> in undo_stack_push(), do I understand it right that when
> the undo stack is full, you discard the last change and replace it
>  with the newest?
yes.

> I like how you created generic listeners for the undo/redo actions. Is
> there a reason why they weren't merged into one type? After all, the
> sheet_delete_para_t and sheet_insert_para_t do not differ that much
> and it would save-up some coding (such as iteration over the
> listeners).
I just want them consistent with parameters of sheet_insert and sheet_delete.

> In undo_stack_pop you set the listen_mask to 0 and then you re-set it
> to both INS and DEL. It would be safer to save the old value and
> restore it in case this function was called with listen_mask already
> modified.
you are right.

> In my humble opinion, I do not think that it is necessary to create
> separate type sheet_insert_listener_link_t just to add link_t. Many
> other structures in HelenOS have link_t as a member to simplify usage
> within lists.
I don't think it a good idea to add a link_t member to any data structure
that can put into a list.
After all, a data structure and a container are logically separated.I don't
want them related so closely.

>To sum it up: your patch is rather long and thus there were more
>things to review and comment on. In general, I like it. I might do
>some modifications prior merging but this will find its way into
>mainline. Thank you very much.
I'm glad to hear that.
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel

Reply via email to