Code review comment for lp://staging/~3v1n0/libindicator/shortcut-path-key

Revision history for this message
Marco Trevisan (TreviƱo) (3v1n0) wrote :

> Thanks for writing this patch. I see one minor problem and also have a
> question:
>
> Needs Fixing:
>
> 1. This patch introduces a side-effect to the function: the cwd is still
> changed when the function returns. It would be safer to remove this side-
> effect by wrapping the block in
>
> char * current_dir = g_get_current_dir();
>
> ...
>
> g_chdir (current_dir);
> g_free (current_dir);

Yes, I thought about that... But I didn't do that as it was the same way as g_spawn* does for example... However, it also sounds better to me. So here it is!

> 2. Putting four g_free() statements on a single line really itches. :-) Please
> use a newline between each.

Yes, right... I just kept the old style.

> 3. Should the chdir() call in the patch be a g_chdir()? I think the answer is
> 'no' but I'm unsure.

It's just the same, and AFAIK we won't use this but in linux, it should be the same :)
(it also would need to include something else).

« Back to merge proposal