_dbus_string_tolower_ascii() was [Commits to core *must* have reviewers]

[Colin Walters]

Hi,

On Wed, Feb 24, 2010 at 9:04 AM, Ralf Habacker <ralf.habacker at freenet.de> wrote:

First, there's various trailing whitespace in the patch.  You might
find my script for
fixing this useful:

http://cgwalters.livejournal.com/25021.html

> +#include <ctype.h>

We can drop this include now.

>
> +    if (_dbus_string_tolower_ascii(&str,0,_dbus_string_get_length(&str)+1))
> +      _dbus_assert_not_reached ("_dbus_string_tolower_ascii failed to check
> boundings");

See my comments on the boolean return value for why I think we should
just delete this check.

> +
> +    if (!_dbus_string_tolower_ascii(&str,0,_dbus_string_get_length(&str)))
> +      _dbus_assert_not_reached ("_dbus_string_tolower_ascii failed");

Inconsistent spacing here; need space between identifier and paren, and after
a comma.  In other words:

if (_dbus_string_tolower_ascii (&str, 0, _dbus_string_get_length (&str)))
  _dbus_assert_not_reached ("_dbus_string_tolower_ascii failed");

But you'll also need to have this stop checking the boolean return too.

> +    fprintf(stderr,_dbus_string_get_const_data(&str));

Leftover debugging fprintf?

> +    if (!_dbus_string_equal_c_str(&str,lower_string))
> +      _dbus_assert_not_reached (_dbus_string_get_const_data(&str));

And here.  Also shouldn't the assertion be like:

  _dbus_assert_not_reached ("_dbus_string_tolower_ascii failed")

>  /* we allow a system header here, for speed/convenience */
>  #include <string.h>
> +#include <ctype.h>

We don't need this include anymore.

+dbus_bool_t
+_dbus_string_tolower_ascii (const DBusString *str,
+                            int               start,
+                            int               len)

Thinking about it I really don't like this function returning a bool
for whether or not the passed in bounds were valid.  Every single
other function in dbus-string.h (and really, DBus in general) uses the
boolean for out of memory.

The range check should just be _dbus_assert I think, and this function
should return void.

> +#if defined(_DEBUG) && defined(DBUS_WIN)
> +  char *dummy1;       /**< placeholder */
> +#else
>   const void *dummy1; /**< placeholder */
> +#endif

Also what is the rationale behind this change?  It seems totally unrelated...