Re: [PATCH 6/9] grep: cache userdiff_driver in grep_source

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 6/9] grep: cache userdiff_driver in grep_source

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:52:55

Jeff King [off-list ref] writes:
-		grep_attr_lock();
-		drv = userdiff_find_by_path(gs->name);
-		grep_attr_unlock();
-		if (drv && drv->funcname.pattern) {
-			const struct userdiff_funcname *pe = &drv->funcname;
+		grep_source_load_driver(gs);
+		if (gs->driver->funcname.pattern) {
+			const struct userdiff_funcname *pe = &gs->driver->funcname;
When we load driver, gs->driver gets at least "default" driver, so we no
longer need to check for drv != NULL as we used to?  Is that the reason
for the slight difference here?
quoted hunk
@@ -1237,6 +1234,7 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
 	gs->name = name ? xstrdup(name) : NULL;
 	gs->buf = NULL;
 	gs->size = 0;
+	gs->driver = NULL;
 
 	switch (type) {
 	case GREP_SOURCE_FILE:

Re: [PATCH 6/9] grep: cache userdiff_driver in grep_source

From: Jeff King <hidden>
Date: 2016-06-15 22:52:55

On Thu, Feb 02, 2012 at 10:34:07AM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
-		grep_attr_lock();
-		drv = userdiff_find_by_path(gs->name);
-		grep_attr_unlock();
-		if (drv && drv->funcname.pattern) {
-			const struct userdiff_funcname *pe = &drv->funcname;
+		grep_source_load_driver(gs);
+		if (gs->driver->funcname.pattern) {
+			const struct userdiff_funcname *pe = &gs->driver->funcname;
When we load driver, gs->driver gets at least "default" driver, so we no
longer need to check for drv != NULL as we used to?  Is that the reason
for the slight difference here?
Yes, exactly.

We could just leave gs->driver NULL instead of looking up "default", and
then use NULL to signal to the calling code that defaults should be
used. But NULL is interpreted by grep_source_load_driver as "we did not
look up the driver yet", so the common case of "no driver" would mean we
accidentally do the lookup multiple times.  The diff_filespec code uses
the same convention to solve the same problem.

Speaking of which, there was some notion in my mind that a "grep_source"
and a "diff_filespec" were very similar objects, and that we could
possibly unify the implementations. I decided against that route with
this series, as it would have involved pretty heavy refactoring of the
diff code to prevent a fairly small amount of code duplication.

-Peff
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help