Re: [htdig-dev] Feature: Execution time

[Gilles D. <gr...@sc...>]

According to Gabriele Bartolini:
> > As far as I know, there isn't anything like that in the code right now,
> > and it does sound like it could be useful, especially in testing.
> 
> I put it in the 3.1.x branch. Please check whether everything sounds
> good for you, and I'll put it into the main branch as well.
> 
> I hope I did not make any disaster when committing to this branch.

Well, technically, the 3.1.x is under a permanent feature freeze,
so any such feature additions, as opposed to bug fixes, should really
go to a vote by the developers.  I know I haven't always followed that
freeze myself, but I've gotten away with this because I was essentially
the one person making or reviewing all changes since 3.1.3.  However,
due to the bugs I inadvertently introduced into 3.1.6 because of all
the new features, it was really my hope that 3.1.7, when I can find the
time to work on it, would be bug fixes only and no new features unless
really sorely needed.

I don't see anything technically wrong with your code, but I am concerned
about the portability of the gettimeofday() call and timeval structure.
Is this sure to be supported on all platforms 3.1.x currently supports,
including Cygwin?

> There are 2 considerations I think we may pay attention to:
> - I included the <sys/time.h> file for the gettimeofday function: should
> we change the configure file?

Well, I was going to suggest at least testing HAVE_SYS_TIME_H, if not
also TIME_WITH_SYS_TIME, as htdig/Document.h and htlib/lib.h do, but
I see that htlib/Connection.cc doesn't check either before including
sys/time.h, so you're not breaking anything that's not already broken.
For the sake of completeness, though, it may be a good idea to fix this
in both htsearch.cc and Connection.cc.

I think a configure test for gettimeofday is pretty much essential, though.

> - I created the template variable EXECUTIONTIME, which displays the time
> in a "seconds.milliseconds" format (like Google). Give a look at the
> Display::setExecutionTime method. If everything is fine, we should put
> it into the documentation as well.
> 
> Please let me know if it works fine. I applied this patch to our Web
> site and it works pretty well and really nicely! :-)

Again, I'm a bit concerned about the portabality of snprintf() in
setExecutionTime().  The db code in 3.1.x seems to go through contortions
to avoid problems with systems that don't have it, and the htdig 3.2 code
seems to go through similar efforts, providing an alternate function in
htlib, for systems that lack it.  If we don't do this in 3.1.x, I think
we're asking for compatibility/portability problems.

So, I need to ask, is this one little feature worth all this extra effort
to support it reliably in 3.1.x?

For now, I vote...

-1

The 3.2 code is another story, as snprintf and gettimeofday are already
used elsewhere in the code (and presumably gettimeofday is provided if
missing, or it's not an issue (yet)), and 3.2 isn't under a feature freeze.
So, go nuts on 3.2, but please tread ever so carefully on the 3.1.x branch.
It's not the place to be testing new features, IMHO, unless you make a good
case for it.

-- 
Gilles R. Detillieux              E-mail: <gr...@sc...>
Spinal Cord Research Centre       WWW:    http://www.scrc.umanitoba.ca/
Dept. Physiology, U. of Manitoba  Winnipeg, MB  R3E 3J7  (Canada)