Created attachment 172322 [details] [review]
Don't pass in errors that then get set and then overwritten

The code was clearing the errors in the case where they weren't set
and the cascading if statements were clobbering existing errors in
the failure cases.

Created attachment 172371 [details] [review]
Make color constants work without warnings

Don't really like that your patch will complain about '0..' as a bad color
value. I think the attached is a bit better - it goes back more to the
way that the code was structured before the bug was originally introduced.

Comment on attachment 172371 [details] [review]
Make color constants work without warnings

>+      /* We don't know how a a constant is going to be used, so we have guess its
>+       * type from its contents:
>+       *
>+       *  - Starts like a number and contains a '.': float constant
>+       *  - Starts like a number and doesn't contain a '.': int constant
>+       *  - Starts with anything else: a color constant.
>+       *    (colors always start with # or a letter)
>+       */

This doesn't seem to be true; parse_positive_integer() accepts named constants in addition to numbers. (And it's probably a bug that parse_double() *doesn't*.)

>+      if (value[0] == '.' || value[0] == '-' || (value[0] >= '0' && value[0] <= '9'))

in theory, a number can also start with '+'

>+          if (strchr (value, '.'))

the existing code would also accept scientific notation floats (1e5), though that's probably unintentional and maybe undesireable.

(In reply to comment #3)
> (From update of attachment 172371 [details] [review])
> >+      /* We don't know how a a constant is going to be used, so we have guess its
> >+       * type from its contents:
> >+       *
> >+       *  - Starts like a number and contains a '.': float constant
> >+       *  - Starts like a number and doesn't contain a '.': int constant
> >+       *  - Starts with anything else: a color constant.
> >+       *    (colors always start with # or a letter)
> >+       */
> 
> This doesn't seem to be true; parse_positive_integer() accepts named constants
> in addition to numbers. (And it's probably a bug that parse_double()
> *doesn't*.)

I'm a little skeptical that it's useful to be able to set a constant to be exactly equal to another constant, but not be able to set a constant to be able to be an expression based on another constant

And the fact that this worked only for integers and colors but not doubles makes me think it was completely accidental. So, I'm willing to risk that no themes were relying on that. (True for the small set of themes on my system.)

> >+      if (value[0] == '.' || value[0] == '-' || (value[0] >= '0' && value[0] <= '9'))
> 
> in theory, a number can also start with '+'

Hmm, didn't know that strtod accepted that.
 
> >+          if (strchr (value, '.'))
> 
> the existing code would also accept scientific notation floats (1e5), though
> that's probably unintentional and maybe undesireable.

The old code was:

-      if (strchr (value, '.') && parse_double (value, &dval, context, error))

I'll push the patch with the addition of '+' support, and hopefully we won't break too many obscure themes that happened to stumble on the specifying-integer-constant-as-integer-constant bug.