List:MySQL++« Previous MessageNext Message »
From:Rick Gutleber Date:November 13 2008 6:19pm
Subject:Re: insertfrom( ) questions
View as plain text  
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.
>

Thread
insertfrom( ) questionsRick Gutleber7 Nov
  • Re: insertfrom( ) questionsWarren Young8 Nov
    • 'result' is not a member of mysqlppJJ Harrison8 Nov
      • Re: 'result' is not a member of mysqlppWarren Young8 Nov
    • Re: insertfrom( ) questionsRick Gutleber10 Nov
      • Re: insertfrom( ) questionsWarren Young11 Nov
        • Re: insertfrom( ) questionsRick Gutleber11 Nov
          • Re: insertfrom( ) questionsWarren Young11 Nov
            • Re: insertfrom( ) questionsRick Gutleber12 Nov
    • Re: insertfrom( ) questionsRick Gutleber13 Nov
      • Re: insertfrom( ) questionsWarren Young13 Nov
        • Re: insertfrom( ) questionsRick Gutleber13 Nov
          • Re: insertfrom( ) questionsWarren Young13 Nov
  • RE: insertfrom( ) questionsIan Daysh11 Nov
    • Re: insertfrom( ) questionsJim Graf11 Nov