Having read through HACKERS.TXT again (it's amazing how many things were
in there that I forgot, including almost all the feedback you gave me on
code style, etc), I have a couple questions and comments:
1. Tests are supposed to be noiseless, but what about qssqls? I wrote
a similar test for the SQLStream class, but now that I've done it, I
don't see where or how it would be used. Also, I added string types to
the SSQLS class, since the reason they were excluded is no longer
relevant. (grep "asplode" in qssqls.cpp), and did the same thing in
qssqls.cpp as well.
2. There's a new test called "array_index.cpp" in mysql.bkl, but no
corresponding file. I removed it for the time being. Was something not
checked in?
3. Using the "pedantic" option in ./bootstrap causes the Makefile to
use a compiler option (-Wno-variadic-macros) that apparently doesn't
exist on the ancient compiler we are using (gcc 3.2.3). I just removed
it from the Makefile, but there may be some autoconf juju that needs to
be tweaked. I honestly didn't want to open that can of worms, and it's
not really a problem, but I thought I'd mention it.
4. I think "asplode" is one of the funniest words ever.
5. Just a final comment. I've really enjoyed working with you and
getting to know this project, and if things work out well, I would be
very interested in continuing to contribute whenever possible. This is
a really well-designed and well-written library, in my estimation, and
I've learned more than I expected from this little escapade.
Rick
Warren Young wrote:
> Rick Gutleber wrote:
>>
>> 1. Does the new insertfrom( ) method and the InsertPolicy and
>> SQLStream objects require anything new in the tests? The tests are
>> very concise; I don't know if they are meant to be exhaustive.
>
> The MySQL++ test suite's concision reflects its immaturity, not a
> design goal. There should be a test for SQLStream at least in tests/*
> and code to exercise insertfrom() in examples/*.
>
> For the example, maybe something that reads a tab-delimited file of
> 'stock' items, makes a vector of SSQLSes from this, then inserts that
> into the DB?
>
>> 2. Do I need to make additions to the user manual to reflect these
>> new pieces? Or is there someone else who handles documentation changes?
>
> Leave userman to me.
>
>> I attached the patch but if there is anything else I need to do, I
>> will make those changes and resubmit.
>
> Yeah, it needs some style fixes.
>
> 1. I don't see that splitting insertfrom() into public and private
> halves buys much. Multiple 'success = false' lines isn't any less
> redundant than multiple 'ac.rollback()' lines. (Not that I think
> you'll have too many method calls on 'ac'...most of them should be at
> the top and bottom of the function.) You also end up with an empty
> transaction if the insert range is empty, due to starting the
> transaction way up high like that, before you test the iterator
> range. If I'm wrong, tell me why.
>
> 2. In the code, use tabs instead of spaces, size 4 [*], and lint your
> patch for other pointless whitespace changes. This accounts for about
> half your patch. As far as I can tell, 100% of the changes to
> lib/query.cpp are of this type, but it happens all through the patch.
> As a result, I haven't really studied the patch carefully. Too much
> noise masking the signal.
>
> 3. Take the unistd.h out of examples/cpool.cpp. I fixed that problem
> in svn a different way. Besides, it's unrelated to this patch.
> (Reading through your patch before submitting it would have caught #2,
> too.)
>
> 4. Say 'svn add lib/sqlstream.* lib/insertpolicy.h' before making the
> next patch. These files are missing from the one you just sent.
>
> 5. Use 0 instead of NULL. (Just style.)
>
> 6. Don't cuddle 'else'.
>
> [*] Yes, we do use spaces instead of tabs in all of the documentation
> files. I've configured my text editor to switch whitespace modes
> based on the file type, so I don't often notice the change any more.
> We may at some point go with spaces across the board, but for the
> nonce, follow the current convention.
>
