[sword-devel] Some Comments on Sword's Code
David White
sword-devel@crosswire.org
13 Feb 2002 21:15:14 +1100
On Wed, 2002-02-13 at 08:00, Troy A. Griffitts wrote:
> David,
> Thank you truely for taking the time to review our code. I appreciate
> your heart to help. My comments below.
No problem; thanks for your response, my comments-to-your-comments
below...
>
>
> On Mon, 2002-02-11 at 04:07, David White wrote:
> > While I was looking through the searching code for Sword, I noticed a
> > few things which seemed little strange, and I thought I'd make comments
> > on ways in which they could be improved:
> >
> > there is lots of code in destructors that looks like this:
> >
> > if (entrybuf)
> > delete [] entrybuf;
> >
> > this is unnecessary, since C++ guarantees that delete [] NULL; will have
> > no effect.
>
> Yes, this is left over from my C days. I understand that most C++
> compilers will catch a delete [] 0, for me, but I don't necessarily
> think it's bad practice to check myself-- especially when I know C does
> not have to handle this for me.
The C++ standard dictates that all compilers must check this. I would be
extremely surprised if any C++ compiler didn't check this, since it is
very easy to implement in the compiler, and would almost certainly cause
immediate and obvious problems with hundreds of programs if it didn't.
I assume when you say that C doesn't have to handle it, you are talking
about the free() function in C. The C89 standard dictates that compilers
must check the argument to free(), and do nothing if it's NULL. It is
true that some pre-standard C compilers did not provide this check.
Even if you did want to check, I would suggest wrapping it in a
function:
template<class T>
void delete_array(T*& p) {
if(p)
delete [] p;
p = 0;
}
This function also has the added benefit of setting the pointer to null
(if it's an l-value), which can be very beneficial in tracking down bugs
etc.
>
>
> > Further, there seems to be a large amount of unnecessary memory
> > allocation and deallocation. For example, entrybuf is allocated as new
> > char[1]; - and as far as I can see it exists for the life of the object,
> > and then is deleted in the destructor; why not just use char
> > entrybuf[1]; ? - it is more efficient, more maintainable and more
> > reliable.
>
> I'm assuming that you mean entrybuf in our data drivers. This variable
> is used to hold the current entry buffer and will be reallocated to the
> appropriate size for each entry.
Well in this case, I assume you have to call delete [] on it, and then
new []. This is inefficient, and I would suggest using a vector instead.
Remember, a vector is really just a very thin wrapper around a resizing
array.
> > Instead of using constructor initialiser lists, variables are
> > initialised inside the body of the constructor; it is generally
> > considered better to initialise variables properly.
>
> I don't believe there is any 'properly' dubbed way to initialize
> variables. To defend my preference to use the body, when we overload
> constructors, we usually move the initialization to a void init() method
> so all constructors use the same initialization. It's much easier to
> move the initialization the way the code stands currently. I don't like
> redundantly initializing variables in each constructor's signature, and
> don't feel this was the purpose of this mechanism. I believe it is
> primarily intended to give a place to specify parameters to complex
> object types that get created upon construction of an object. We mostly
> have POINTERS to complex objects. This means we don't need to
> initialize a bunch of complex objects until they are needed.
Admittedly this is a fairly minor point, but in general it is a good
idea to make sure an object is in a nice, consistent state, from the
time it is initialised to the time it is destructed. This means that all
data members and pointers in particular should have sane, legal values.
Once you enter the constructor's body, you have entered the code of the
class, code which can potentially call class members, or access and
dereference the class's data and pointer members. By using an
initialiser list, you can avoid this potential problem in a cleaner,
safer way.
But yes, this isn't all that big a deal, just as long as you make sure
all POD types are given sane values by the constructor at some point.
>
>
> > The search function, takes a parameter "int searchType", which is
> > documented to be a regex search if search is >= 0, a phrase if search is
> > -1, and a multiword search if phrase is -2. It would be alot cleaner,
> > and typesafe just to use an enumeration:
> >
> > enum SearchType { REGEX_SEARCH, MULTIWORD_SEARCH, PHRASE_SEARCH };
>
> Good suggestion, thank you. The history of the signature mandated this
> construct. The first parameter used to be REGEX_ params. These were
> all >= 0 When we added a new parameter to the method, we kept backward
> compatability by making the 2 new search types <0. You may still pass
> REGEX_ params in this param, but it has been some time since this change
> and we should probably move to typed parameter like your enum.
ok, I see why it was done like that now, although perhaps this should be
documented in the description; I don't see anything about being able to
use REGEX_ params...
>
>
> > Search also takes a pointer to a function, "void (*percent)(char, void
> > *)". It would be more flexible to make the function templated, and make
> > percent a function or function-like object that is passed in. That way,
> > the percentUserData parameter would also be unnecessary, as the user
> > could pass in a functoid which stores data if that is the desired
> > behaviour.
>
> IMO, most people would not be able to use this complex mechanism, and is
> overkill.
Perhaps I didn't explain it very well, but I see this way of doing it as
simpler, more flexible and more robust. I can certainly tell you that
pointers-to-functions confuse me, and no doubt other people as well.
Many people will have no idea what the void* parameter is, and it will
confuse them. What I am suggesting is basically:
template<class PercentCallback>
...::Search(...., PercentCallback percent)
{
...
percent(1);
...
percent(2);
...
}
The user can use this functionality very easily:
void PercentDone(int percent) {
//functionality in here...
}
...
module.Search(..., PercentDone);
This is simpler and easier, because the user doesn't have to worry about
a parameter if they aren't interested in one, and they won't have to
call a reinterpret_cast if they do want one. If for example, they had a
class called ProgressBar and they wanted a member function called
SetPercent(int) to be called, then all they would have to do is this:
ProgressBar bar;
...
module.Search(..., bind1st(mem_fun1_ref(&ProgressBar::SetPercent),
bar));
Yes, that is a little more complex, but it is something which is not
possible at all with the current approach. It is also alot safer, since
it does not require the user to reinterpret_cast a pointer-to-void. Less
experienced users can still use the first example.
For people who want to understand how this works, mem_fun1_ref takes a
member function which takes one argument, and returns a two-argument
non-member function, the first argument being a reference to an object
of the type the member function is a member of. bind1st takes a
two-argument function, f(x,y), and an object n, and returns a
one-argument function g, such that g(y) = f(n,y).
Of course, it would be quite trivial to write a backward-compatible
wrapper for this function; the same holds for most of my suggestions.
>
>
> > There is alot of code that could be written better using the STL, for
> > example:
> >
> > for (unsigned int i = 0; i < strlen(wordBuf); i++)
> > wordBuf[i] = toupper(wordBuf[i]);
> >
>
> Yes, strlen should not be in the condition of the for loop. I did find
> this construct in a few places in our library.
>
> The rest of your message seems to suggest using STL throughout the
> engine.
Absolutely; that's precisely what I think would be good to do.
> Although I am not against using STL in places where it is
> obviously beneficial, I don't agree that it is mostly more efficient.
> Here is a simple example.
The STL is not inherently more efficient, however I believe it provides
mechanisms that allows one to more easily write more efficient (and
robust, reliable, flexible and maintainable) code.
> Compile these 2 programs:
>
> yoyo.cpp:
> _____________
> #include <string.h>
>
> int main(int argc, char **argv) {
> const char *sample[] = {"1", "12", "123", "1234", "12345",
> "123456", "1234567", "12345678", "123456789", "1234567890"};
> for (long i = 0; i < 9999999L; i++) {
> char *stuff = 0;
> stuff = new char [ strlen(sample[i%10]) ];
> strcpy(stuff, sample[i%10]);
> stuff; // access buffer
> delete [] stuff;
> }
> return 0;
> }
>
> ______________________
>
>
> yoyo2.cpp:
> ______________
> #include <string>
>
> int main(int argc, char **argv) {
> const char *sample[] = {"1", "12", "123", "1234", "12345",
> "123456", "1234567", "12345678", "123456789", "1234567890"};
>
> for (long i = 0; i < 9999999L; i++) {
> string stuff;
> stuff = sample[i%10];
> stuff.c_str(); // access buffer
> }
> return 0;
> }
>
>
> On my system, without optimization, yoyo runs 2x faster. With
> optimization yoyo2 runs 1.2x faster.
My system had fairly similiar results. However, I think there were a
couple of things in your test that could be improved. Firstly, you are
calling c_str() on the string in yoyo2, this doesn't just "access the
buffer", it produces a pointer to a c-style string. If you use the stl
consistently, you shouldn't have to call c_str() too often. A fairer
test is to call data() or stuff[0]; I changed it to do this, and it cut
off about 20% of the running time.
In yoyo, you are doing an unnecessary iteration over the string. You
already call strlen(), so you know the length of the string, so why not
call memcpy() instead of strcpy()? I changed it to do this, and it cut
off about 10% of the running time.
Now, your example has presented me with an excellent opportunity to
demonstrate how easy it is to optimise STL-using programs in many cases.
If you put the declaration of stuff outside the for loop:
string stuff;
for(...) {
stuff = sample...
the functionality is exactly the same, yet another 30% or so of running
time is shaved off, due to unnecessary allocation not having to happen
over and over. Of course, this optimisation working in this contrived
example doesn't mean it's generally appliable, but it is a good example
of how optimising STL-based programs is often easy. It would be
significantly more complex to produce a similiar optimisation for
yoyo.cpp.
Now, I have written an alternative program, which uses vectors instead
of strings. I don't see any reason to allocate unwrapped arrays on the
heap in C++, you might not always find std::string fast enough to
fulfill your needs, that's fine - just use a faster mechanism:
yoyo3
----------
#include <algorithm>
#include <vector>
int main() {
const char *sample[] = {"1", "12", "123", "1234", "12345",
"123456", "1234567", "12345678", "123456789", "1234567890"};
for (long i = 0; i < 9999999L; i++) {
const char* const s = sample[i%10];
vector<char> stuff(strlen(s)+1);
copy(s, s+stuff.size(), stuff.begin());
&stuff[0]; // access buffer
}
}
yoyo ran about 1.5x as fast as this with optimisations switched off, but
yoyo3 ran about 1.5x as fast with optimisations switched on.
If you wanted, you could optimise either of the STL versions
considerably. The best way to do this would be to write a custom
allocator that recycles memory, or uses some memory that is allocated on
the stack. It would be much harder to optimise yoyo.
Now, another point to consider is that yoyo is not exception-safe. That
is, if "access buffer" contains something that throws an exception, what
is going to happen to the memory allocated? It will be leaked. (of
course you could put try...catch clauses around everything, but that's
just painful....)
> I was suprised that yoyo2 ran
> faster with optimization. This gives me a much better impression of
> STL.
Yes, with a half-decent compiler, the STL produces very fast code. It
often "looks" like it'll be slower, because it looks much higher level,
but in fact it produces code competitive with C-style programs.
> Either case, stripped binaries of yoyo2 are 2x larger.
Well, with optimisations turned on, yoyo2 was only about 30% larger than
yoyo, and yoyo3 was only about 20% larger than yoyo. Also, in a large
project, this is going to be amortised across the entire project. I
don't think it's that big an issue, especially considering how heavily
the size-speed tradeoff has been skewed towards speed. I'm sure you
could even just play with some compiler switches to turn off some
inlining and trade some size for speed...
> This does
> not take into account passing complex objects on the stack.
What do you mean by this?
In an
> engine used by many frontends, I feel speed and size are more important.
I would have said that reliability and robustness is most important.
Flexibility and speed might come equal second, as for size...well I
think that'd be somewhere way down the list. Oh and yeah, portability
would be fairly high up there somewhere too.
> Not using STL in many cases gives us the opportunity to optimize code
> rather than being at the mercy of the compiler's optimization.
Really? People who write compilers and STL implementations understand
all the nasty details of how their platforms work, they know how to
optimise code to work nicely with their architecture's page size,
preferred allocation scheme etc. I don't. I would far prefer to leave it
up to them, and concentrate on the logic of what my program is actually
trying to do, rather than having to worry about whether it's better to
reallocate linearly or exponentially, and what buffer my string is going
to end up in.
I also managed to try your examples on my Windows machine at work -
using VC++. It showed fairly similiar results. It seems to me that
STL-based stuff will run extremely well on any decent compiler/platform.
[snip]
>
> The rest of your suggestions below are valuable, also. Thank you. I
> would love to have someone with your knowledge of C++ to be contributing
> code. A 9 year evolution of the API has produced inconsistent and
> confusing code in some areas. After 1.5.4, we plan to begin 1.99.x,
> which will include a rewrite of much of the API looking towards a 2.0.0
> release. Please consider being involved in that process.
I didn't realise Sword was 9 years old. Was it originally written in
C++, or just C? I am surprised it is as consistent as it is, being that
old.
I would like to become involved, although unfortunately I wouldn't
necessarily have alot of time. If I did want to, what are the procedures
for submitting of changes? Are proposed changes just discussed here, or
do one or more people have to approve them?
---
Just one other thing I noticed, often operator++ is defined like this:
Object& operator++(int) {
return *this += 1;
}
this is incorrect semantics for the post-increment operator. It is meant
to return the state of the object before iteration, not after. Also, you
don't provide a pre-increment operator. The correct way to do it is:
Object& operator++() {
return *this += 1;
}
Object operator++(int) {
const Object temp = *this;
operator++();
return temp;
}
Yes, it looks a little weird; because of this, it's also less efficient
to use postincrement than preincrement for non-pod types. Unless you
store the return value, it's better to use preincrement.
God Bless,
David.
>
> -Troy.
>
>
> > There are numerous calls to C-style allocation functions (calloc and
> > realloc), I would suggest considering using the more typesafe and
> > powerful std::vector. In particular, it is very difficult to make
> > C-style allocated code exception-safe, and if you really have to use it,
> > I would suggest using an auto_array class, in the spirit of
> > std::auto_ptr. Also, a problem with C-style allocation, is it makes it
> > even more difficult to deallocate memory properly, since you have to be
> > careful whether to use free() or delete or delete [], corresponding to
> > whether the memory was allocated using malloc/calloc/realloc, new, or
> > new []. Worse, on many compilers, allocating with new [] and freeing
> > with free() is fine, but only when you port to another platform will you
> > find lots of weird bugs.
>
>
> >
> > I also noticed lines that looked like this:
> >
> > indexes.erase(indexes.begin(), indexes.end());
> >
> > this should be: indexes.clear();
> >
> > I also noticed, that other than dynamic casts, Sword seems to
> > exclusively use C-style casts. I would recommend not using C-style
> > casts, as they are more dangerous, and might do an unexpected const_cast
> > or reinterpret_cast - both of which can easily cause undefined
> > behaviour. Generally, try to use static_casts instead of C-style casts.
> >
> > Sword often uses the statement using namespace std; It is generally
> > considered bad style to import an entire namespace, particularly in a
> > header file. Safer is to just import the names you actually use as in,
> > using std::list; or simply explicitly qualify all uses of names. I
> > prefer the latter option personally, although I understand use of the
> > former.
> >
> > This is getting rather voluminous, so I think I'll stop now. I apologise
> > if any of my comments seem like unnecessary criticisms, they are
> > intended rather to be helpful comments on how to improve the quality of
> > the code base. I can assure you that the code to my program is far worse
> > than Sword's code (I really had no idea what I was doing when I first
> > started writing it). Also, some of my above comments may be invalid,
> > since use of a subset of C++'s features may be intentional to allow
> > portability to some platforms with poor C++ support.
> >
> > I would be happy to comment further if any of my comments have proved
> > helpful or instructive.
> >
> > God Bless,
> >
> > David.
>
>