Re: Re: [evolution-patches] Bounty Hunt patch: Set wallpaper from mailer

[Jeffrey Stedfast <fejj ximian com>]

On Tue, 2003-12-02 at 23:46, David Moore wrote:
> > Actually this really isn't a problem, it just returns a pointer (the actual name saved) vs bool.  This implicitly supplies as much info as the bool, plus useful information too, the saved filename.  The ononly difference is you have to save the return, and g_free it - not a big change.  The harder part is save_part.
> 
> I'm not sure I understand what you're asking. Should the number-appending code be in save_part or in set_background? 
> 
> > don't add this whitespace stuff.  i preferred the "name(" function calls, like k&r taught me.  
> 
> Okay. The coding style guidelines at developer.ximian.com/projects/evolution/patch_guidelines.html show it with the whitespace, though.
> 
> > you really can't do this, it assumes you have an extension, but you could have anything given that the value most likely came from the internet.-> crash if there's no '.' in the name.
> 
> No, it works out. strrchr returns NULL if '.' is not found, and g_strdup returns NULL if passed NULL. So if there's no '.', the if(!extension) evaluates to true and extension is set to "". I could be missing something, but it works for me.
> 

don't strdup the extension - it's unnecessary and you also have a bug
there, since your code sets extension = ""; if strdup returns NULL and
then g_free (extension) later which is a Bad Thing (tm) ("" was not
malloc'd).

also, basename can be calculated thusly:

basename = extension ? g_strndup (filename, extension - filename) :
g_strdup (filename);

actually, I'd probably just do:

if ((extension = strrchr (filename, '.')))
   basename = g_strndup (filename, extension - filename);
else
   basename = g_strdup (filename);

I think what Michael means is that you can't expect the .* to
necessarily be a file-type extension. eg. "my.grocery.list" is a valid
filename, but ".list" is not a file-type extension. not sure it really
matters in this situation tho.

> > The above can be done in one loop, without having the encompassing if() {}
> 
> Maybe I'm misunderstanding you, but removing the if() would result in extension and basename being calculated and then immediately freed when they're not needed, which is kind of a waste. 
>

they are only free'd right away if the file already exists, which is
exactly what your code is doing, albeit in a more complicated way :-)

besides, you can calculate the extension and basename before the
while-loop. only ever needs to be done once afterall.

you can even optimise things more by not strduping the basename (doesn't
really need to be done).

one way to do it might be:

const char *p, *ext = NULL;
unsigned int i = 1;
char *fullpath;

for (p = filename; *p; p++) {
   if (*p == '.')
      ext = p;
}

fullpath = g_strdup (filename);

while (file_test (fullpath)) {
   g_free (fullpath);
   fullpath = g_strdup_printf ("%.*s(%u)%s", ext - filename, i++, ext);
}


now, if you are really worried about performance of the loop with all
the strduping and such... do this:

fullpath = g_malloc (strlen (filename) + 10 + 3);
strcpy (fullpath, filename);

while (file_test (fullpath))
   sprintf (fullpath, "%.*s(%u)%s", ext - filename, i++, ext);


Jeff

> _______________________________________________
> Evolution-patches mailing list
> Evolution-patches lists ximian com
> http://lists.ximian.com/mailman/listinfo/evolution-patches
>