Re: [PATCH 2/9] strbuf: add strbuf_tolower function
On Thu, May 22, 2014 at 03:52:08PM -0700, Kyle J. McKay wrote: Christian brought this up elsewhere, and I agree it's probably better to work over the whole buffer, NULs included. I'm happy to re-roll (or you can just pick up the version of the patch in this thread), The only reason I brought up the code difference in the first place was that the comment was This makes config's lowercase() function public which made me expect to see basically the equivalent of replacing a static with an extern, but actually the function being made public was a different implementation than config's lowercase() function. So it looks like the original PATCH 2/9 version of the code is best, but perhaps the comment can be tweaked a bit to not convey the same impression I got. Maybe something like Replace config's lowercase() function with a public one. Yeah, sorry if it sounded like I was complaining about your review elsewhere. I mostly found it amusing that I got two opposite-direction reviews. I agree that clarifying the situation in the commit message is best, and I've done that in the version I just posted. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] strbuf: add strbuf_tolower function
On May 23, 2014, at 13:05, Jeff King wrote: On Thu, May 22, 2014 at 03:52:08PM -0700, Kyle J. McKay wrote: Christian brought this up elsewhere, and I agree it's probably better to work over the whole buffer, NULs included. I'm happy to re-roll (or you can just pick up the version of the patch in this thread), The only reason I brought up the code difference in the first place was that the comment was This makes config's lowercase() function public which made me expect to see basically the equivalent of replacing a static with an extern, but actually the function being made public was a different implementation than config's lowercase() function. Yeah, sorry if it sounded like I was complaining about your review elsewhere. I mostly found it amusing that I got two opposite-direction reviews. I didn't seem like complaining to me. I also was amused. :) I agree that clarifying the situation in the commit message is best, and I've done that in the version I just posted. It looks great. And I suspect you're correct that a modern compiler would optimize the index-based version to be as efficient as the pointer arithmetic version. --Kyle -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] strbuf: add strbuf_tolower function
Jeff King p...@peff.net writes:
On Wed, May 21, 2014 at 05:07:36PM -0700, Kyle J. McKay wrote:
+void strbuf_tolower(struct strbuf *sb)
+{
+ size_t i;
+ for (i = 0; i sb-len; i++)
+ sb-buf[i] = tolower(sb-buf[i]);
+}
+
Wouldn't a direct transfer of the lowercase function be something more like:
void strbuf_tolower(struct strbuf *sb)
{
char *p = sb-buf;
for (; *p; p++)
*p = tolower(*p);
}
That seems to me to be a bit more efficient. According to the comments in
strbuf.c, people can always assume buf is non NULL and -buf is NUL
terminated even for a freshly initialized strbuf.
Yes, and that would be fine with me (I actually wrote strbuf_tolower for
my own use, and _then_ realized that we already had such a thing that
could be replaced).
Do we forbid that sb-buf[x] for some x sb-len to be NUL, and if
there is such a byte we stop running tolower() on the remainder?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] strbuf: add strbuf_tolower function
On Thu, May 22, 2014 at 11:36:37AM -0700, Junio C Hamano wrote: Yes, and that would be fine with me (I actually wrote strbuf_tolower for my own use, and _then_ realized that we already had such a thing that could be replaced). Do we forbid that sb-buf[x] for some x sb-len to be NUL, and if there is such a byte we stop running tolower() on the remainder? Christian brought this up elsewhere, and I agree it's probably better to work over the whole buffer, NULs included. I'm happy to re-roll (or you can just pick up the version of the patch in this thread), but I think the bigger question is: is this refactor worth doing, since there is only one caller? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] strbuf: add strbuf_tolower function
Jeff King p...@peff.net writes: Yes, and that would be fine with me (I actually wrote strbuf_tolower for my own use, and _then_ realized that we already had such a thing that could be replaced). ... ... I think the bigger question is: is this refactor worth doing, since there is only one caller? If you wrote it for your own use and then realized that it is applicable to this codepath, wouldn't that say that there are multiple potential callers that would benefit from having it? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] strbuf: add strbuf_tolower function
On May 22, 2014, at 11:41, Jeff King wrote: On Thu, May 22, 2014 at 11:36:37AM -0700, Junio C Hamano wrote: Yes, and that would be fine with me (I actually wrote strbuf_tolower for my own use, and _then_ realized that we already had such a thing that could be replaced). Do we forbid that sb-buf[x] for some x sb-len to be NUL, and if there is such a byte we stop running tolower() on the remainder? Christian brought this up elsewhere, and I agree it's probably better to work over the whole buffer, NULs included. I'm happy to re-roll (or you can just pick up the version of the patch in this thread), The only reason I brought up the code difference in the first place was that the comment was This makes config's lowercase() function public which made me expect to see basically the equivalent of replacing a static with an extern, but actually the function being made public was a different implementation than config's lowercase() function. So it looks like the original PATCH 2/9 version of the code is best, but perhaps the comment can be tweaked a bit to not convey the same impression I got. Maybe something like Replace config's lowercase() function with a public one. --Kyle -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/9] strbuf: add strbuf_tolower function
This makes config's lowercase() function public.
Note that we could continue to offer a pure-string
lowercase, but there would be no callers (in most
pure-string cases, we actually duplicate and lowercase the
duplicate).
Signed-off-by: Jeff King p...@peff.net
---
This ones gets used later on...
Documentation/technical/api-strbuf.txt | 4
config.c | 8 +---
strbuf.c | 7 +++
strbuf.h | 1 +
4 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/Documentation/technical/api-strbuf.txt
b/Documentation/technical/api-strbuf.txt
index 3350d97..8480f89 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -125,6 +125,10 @@ Functions
Strip whitespace from the end of a string.
+`strbuf_tolower`::
+
+ Lowercase each character in the buffer using `tolower`.
+
`strbuf_cmp`::
Compare two buffers. Returns an integer less than, equal to, or greater
diff --git a/config.c b/config.c
index a30cb5c..03ce5c6 100644
--- a/config.c
+++ b/config.c
@@ -147,12 +147,6 @@ int git_config_include(const char *var, const char *value,
void *data)
return ret;
}
-static void lowercase(char *p)
-{
- for (; *p; p++)
- *p = tolower(*p);
-}
-
void git_config_push_parameter(const char *text)
{
struct strbuf env = STRBUF_INIT;
@@ -180,7 +174,7 @@ int git_config_parse_parameter(const char *text,
strbuf_list_free(pair);
return error(bogus config parameter: %s, text);
}
- lowercase(pair[0]-buf);
+ strbuf_tolower(pair[0]);
if (fn(pair[0]-buf, pair[1] ? pair[1]-buf : NULL, data) 0) {
strbuf_list_free(pair);
return -1;
diff --git a/strbuf.c b/strbuf.c
index ee96dcf..a1b8a47 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -106,6 +106,13 @@ void strbuf_ltrim(struct strbuf *sb)
sb-buf[sb-len] = '\0';
}
+void strbuf_tolower(struct strbuf *sb)
+{
+ size_t i;
+ for (i = 0; i sb-len; i++)
+ sb-buf[i] = tolower(sb-buf[i]);
+}
+
struct strbuf **strbuf_split_buf(const char *str, size_t slen,
int terminator, int max)
{
diff --git a/strbuf.h b/strbuf.h
index 39c14cf..6b6f745 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -45,6 +45,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t
len)
extern void strbuf_trim(struct strbuf *);
extern void strbuf_rtrim(struct strbuf *);
extern void strbuf_ltrim(struct strbuf *);
+extern void strbuf_tolower(struct strbuf *sb);
extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);
/*
--
2.0.0.rc1.436.g03cb729
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] strbuf: add strbuf_tolower function
On May 21, 2014, at 03:27, Jeff King wrote:
This makes config's lowercase() function public.
Note that we could continue to offer a pure-string
lowercase, but there would be no callers (in most
pure-string cases, we actually duplicate and lowercase the
duplicate).
Signed-off-by: Jeff King p...@peff.net
---
This ones gets used later on...
Documentation/technical/api-strbuf.txt | 4
config.c | 8 +---
strbuf.c | 7 +++
strbuf.h | 1 +
4 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/
technical/api-strbuf.txt
index 3350d97..8480f89 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -125,6 +125,10 @@ Functions
Strip whitespace from the end of a string.
+`strbuf_tolower`::
+
+ Lowercase each character in the buffer using `tolower`.
+
`strbuf_cmp`::
Compare two buffers. Returns an integer less than, equal to, or
greater
diff --git a/config.c b/config.c
index a30cb5c..03ce5c6 100644
--- a/config.c
+++ b/config.c
@@ -147,12 +147,6 @@ int git_config_include(const char *var, const
char *value, void *data)
return ret;
}
-static void lowercase(char *p)
-{
- for (; *p; p++)
- *p = tolower(*p);
-}
-
void git_config_push_parameter(const char *text)
{
struct strbuf env = STRBUF_INIT;
@@ -180,7 +174,7 @@ int git_config_parse_parameter(const char *text,
strbuf_list_free(pair);
return error(bogus config parameter: %s, text);
}
- lowercase(pair[0]-buf);
+ strbuf_tolower(pair[0]);
if (fn(pair[0]-buf, pair[1] ? pair[1]-buf : NULL, data) 0) {
strbuf_list_free(pair);
return -1;
diff --git a/strbuf.c b/strbuf.c
index ee96dcf..a1b8a47 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -106,6 +106,13 @@ void strbuf_ltrim(struct strbuf *sb)
sb-buf[sb-len] = '\0';
}
+void strbuf_tolower(struct strbuf *sb)
+{
+ size_t i;
+ for (i = 0; i sb-len; i++)
+ sb-buf[i] = tolower(sb-buf[i]);
+}
+
Wouldn't a direct transfer of the lowercase function be something more
like:
void strbuf_tolower(struct strbuf *sb)
{
char *p = sb-buf;
for (; *p; p++)
*p = tolower(*p);
}
That seems to me to be a bit more efficient. According to the
comments in strbuf.c, people can always assume buf is non NULL and -
buf is NUL terminated even for a freshly initialized strbuf.
struct strbuf **strbuf_split_buf(const char *str, size_t slen,
int terminator, int max)
{
diff --git a/strbuf.h b/strbuf.h
index 39c14cf..6b6f745 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -45,6 +45,7 @@ static inline void strbuf_setlen(struct strbuf
*sb, size_t len)
extern void strbuf_trim(struct strbuf *);
extern void strbuf_rtrim(struct strbuf *);
extern void strbuf_ltrim(struct strbuf *);
+extern void strbuf_tolower(struct strbuf *sb);
extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);
/*
--
2.0.0.rc1.436.g03cb729
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] strbuf: add strbuf_tolower function
On Wed, May 21, 2014 at 05:07:36PM -0700, Kyle J. McKay wrote:
+void strbuf_tolower(struct strbuf *sb)
+{
+size_t i;
+for (i = 0; i sb-len; i++)
+sb-buf[i] = tolower(sb-buf[i]);
+}
+
Wouldn't a direct transfer of the lowercase function be something more like:
void strbuf_tolower(struct strbuf *sb)
{
char *p = sb-buf;
for (; *p; p++)
*p = tolower(*p);
}
That seems to me to be a bit more efficient. According to the comments in
strbuf.c, people can always assume buf is non NULL and -buf is NUL
terminated even for a freshly initialized strbuf.
Yes, and that would be fine with me (I actually wrote strbuf_tolower for
my own use, and _then_ realized that we already had such a thing that
could be replaced).
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
