List:MySQL++« Previous MessageNext Message »
From:Warren Young Date:September 26 2007 8:24pm
Subject:Re: Error num in BadQuery patch
View as plain text  
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.)
Thread
RELEASE: v2.3.2Warren Young11 Jul
  • Getting errnum() in exception?Jim Wallace20 Sep
    • Re: Getting errnum() in exception?Warren Young22 Sep
  • Building the head of SVN with MSVC?Jim Wallace25 Sep
    • Re: Building the head of SVN with MSVC?Warren Young26 Sep
  • Error num in BadQuery patchJim Wallace25 Sep
    • RE: Error num in BadQuery patchJim Wallace26 Sep
      • Re: Error num in BadQuery patchWarren Young26 Sep
    • Re: Error num in BadQuery patchWarren Young26 Sep
  • v2.3.2 and execute?Jim Wallace5 Oct
    • Re: v2.3.2 and execute?Warren Young5 Oct
  • Building svn tip on WindowsJim Wallace24 Oct
    • Re: Building svn tip on WindowsWarren Young25 Oct
  • Patch for better exception use -- BadQuery w/Errnum patch part 1Jim Wallace24 Oct
  • Better exception usage -- BadQuery w/Errnum patch part 2Jim Wallace24 Oct
  • Expose errnum() in query -- BadQuery w/Errnum patch part 3Jim Wallace24 Oct
    • Re: Expose errnum() in query -- BadQuery w/Errnum patch part 3Warren Young25 Oct
      • RE: Expose errnum() in query -- BadQuery w/Errnum patch part 3Jim Wallace25 Oct
  • BadQuery w/Errnum patch part 4Jim Wallace24 Oct
    • Re: BadQuery w/Errnum patch part 4Warren Young25 Oct
      • RE: BadQuery w/Errnum patch part 4Jim Wallace25 Oct
  • Sample files -- BadQuery w/Errnum patch part 4Jim Wallace24 Oct
    • Re: Sample files -- BadQuery w/Errnum patch part 4Warren Young25 Oct
      • RE: Sample files -- BadQuery w/Errnum patch part 4Jim Wallace25 Oct
        • Re: Sample files -- BadQuery w/Errnum patch part 4Warren Young25 Oct
          • RE: Sample files -- BadQuery w/Errnum patch part 4Jim Wallace25 Oct
          • RE: Sample files -- BadQuery w/Errnum patch part 4Jim Wallace25 Oct
            • Re: Sample files -- BadQuery w/Errnum patch part 4Warren Young25 Oct
              • RE: Sample files -- BadQuery w/Errnum patch part 4Jim Wallace25 Oct
      • RE: Sample files -- BadQuery w/Errnum patch part 4Jim Wallace25 Oct