Subject: Re: Error num in BadQuery patch 2007-09-26 - By Warren Young
Back
Jim Wallace wrote:
>
> any constructive criticism is welcome.
Oooh, you asked for it. :)
> + throw BadQuery(error(),errnum());
When submitting patches to any project, not just MySQL++, follow the
coding conventions you see in the surrounding code. In this instance,
there needs to be a space after commas.
> class MYSQLPP_EXPORT BadQuery : public Exception
> {
> public:
> - /// \brief Create exception object, taking C string
> - explicit BadQuery(const char* w = "") :
> - Exception(w)
> + int errnum; ///< error number associated with execption
Hide this data member behind an accessor, make it private, and add an
underscore to the end of its name to indicate that it's private. In
other words, treat it just like Exception::what_. This is just another
instance of following existing coding conventions.
> + /// \brief Create exception object, taking C string and error
> number
The briefer doxygen comment made sense with just one parameter, but with
two, you should document both explicitly. I'd also document the reason
why the error number is included, probably using your Transaction
example; the docs should answer the question, "why would I use this
instead of calling Connection::errnum()?"
Ditto for the C++ string case.
> + explicit BadQuery(const char* w = "", int err = 0) :
> + Exception(w), errnum(err)
Variable initializations go on separate lines in the coding style used
for MySQL++.
Also, all of the changes to BadQuery also need to be made to
ConnectionFailed and DBSelectionFailed. It's the same issue. And
therefore, the same patch.
> --- query.cpp 2007-07-11 17:37:16.000000000 -0400
> +++ query.cpp 2007-09-25 09:05:12.000000000 -0400
> @@ -81,13 +81,12 @@
> my_ulonglong
> Query::affected_rows() const
> {
> return conn_->affected_rows();
> }
>
> -
> std::string
> Query::error()
> {
> return conn_->error();
> }
This is a bogus patch hunk. Examine a patch's diff carefully before
submitting it to remove pointless little changes like this. You might
be surprised how often you catch things like this.
> + /// \brief Get the last error number.
> + ///
> + /// This just delegates to Connection::errnum(). Query has
> nothing
> + /// extra to say, so use either, as makes sense in your program.
> + /// If you want the string *and* number you must
> + /// call errnum() before calling error() since error() clears
> errnum()
> + int errnum() const { return conn_->errnum(); }
> +
This is orthogonal to the BadQuery changes, so it should be a separate
patch. The first patch would call conn_->errnum() instead of
Query::errnum(). The second would add this method and change all
conn_->errnum() instances inside Query to just errnum(). This keeps the
revision history clean, and ensures that you can separate effects due to
each functional change.
Another way to think about this is, if you were writing the svn checkin
message, would it have to be a list, or a paragraph, or a run-on
sentence? If so, the patch is probably trying to do too many things at
once. This discipline helps you think more clearly about what each
change is doing.
> if (!result_) {
> if (throw_exceptions()) {
> - throw BadQuery("Results not fetched");
> + throw BadQuery("Results not
> fetched",ER_UNKNOWN_ERROR);
I think we need a new exception type here, not arm-twisting the current
design to take a bogus error number. Historically, BadQuery was grossly
overused; it was essentially a generic exception back in the 1.7 days.
I think this is just a remnant of this problem.
I suggest calling it UseQueryError.
In keeping with earlier advice, I'd make this a separate patch, too. It
should contain the new exception type defn and change the two BadQueries
that don't take mysql_error() as the message to use it instead.
> if (!d || !r) {
> if (throw_exceptions()) {
> - throw BadQuery("ROW or RES is NULL");
> + throw BadQuery("ROW or RES is
> NULL",ER_INVALID_USE_OF_NULL);
> }
> else {
> return;
> }
> }
The MySQL error constant you're misusing here refers to SQL NULLs, not C
NULLs. Also, it's another historical misuse of BadQuery. Use the
ObjectNotInitialized exception here instead. (This is also a separate
patch.)
--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsub=mysql@(protected)
|
|
