--------------080408080603010106030602
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

I'm thinking I might have made a design mistake in the name of
expediency back in v1.7.10 when I fixed the operator[] overloading
problem.

For those who don't remember the "what and why" of it, the problem was
that Row:perator[] was overloaded for several types of integer, for
const char*, and I think even std::string. Since 0 can be converted to
any of those types implicitly, GCC 3 rightly refused to compile any code
saying something like this:

Row r;
cout << r[0] << ....

GCC also gave ambiguous overload errors for code like this:

cout << r["fred"] ...

since "fred" is a pointer, and thus is convertible to an integer.

So, I removed all but operator[](size_type), added
Row::lookup_by_name(const char*) to handle the string cases, and called
it good.

But recently, it came to me that the integer overloads were probably the
least useful of the bunch in real-world code. Intelligent database
design doesn't make the client code dependent on the number or order of
database columns. So, I wonder if it would have been better to keep
operator[](const char*) instead.

Obviously I can't change it back until v1.8 or v2.0. Keeping that in
mind, my question is, who would it bother if I changed these semantics?
Who uses integer indexes into a Row object in real-world code?

I sure don't, but then, I use the SSQLS mechanism, and it hides the Row
access within its internals. Several of the examples use
Row:perator[](int), but they probably shouldn't, for the intelligent
design reason I gave above.

I've attached a diff of the change you'd need to make to get the library
itself and the examples to compile. Notice that class Row can no longer
derive from the template const_subscript_container, since that demands
that the subclass implement operator[](size_type). That's no doubt why
I chose to keep the overload that I did. It's not a very good reason,
but there it is.

I'd appreciate it if some of you would try this patch, and see how it
affects your code. That will help me to decide if this item even makes
it onto the Wishlist. Also, it would be nice to know what other items
from template const_subscript_container we need to hoist up in to class
Row in order to make real-world code continue to compile. I just
hack-and-slashed it for this proof of concept.

--------------080408080603010106030602
Content-Type: text/plain;
name="rowindex.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="rowindex.patch"

Index: examples/complic1.cpp
================================================== =================
RCS file: /usr/local/cvs/mysql++/examples/complic1.cpp,v
retrieving revision 1.2
diff -u -r1.2 complic1.cpp
--- examples/complic1.cpp 1 Mar 2005 03:48:40 -0000 1.2
+++ examples/complic1.cpp 5 Apr 2005 17:04:12 -0000
@@ -43,13 +43,13 @@
// there is a problem in the conversion it will throw an
// exception (caught below). To test it, try changing the
// row[2] to row[0] below.
- cout << setw(17) << row.lookup_by_name("ITEM") <<
- setw(4) << row[1] <<
- setw(7) << double(row[2]) <<
- setw(7) << double(row[3]);
+ cout << setw(17) << row["ITEM"] <<
+ setw(4) << row.col_num(1) <<
+ setw(7) << double(row.col_num(2)) <<
+ setw(7) << double(row.col_num(3));

// The ColData is implicitly converted to a date here.
- Date date = row.lookup_by_name("SDATE");
+ Date date = row["SDATE"];
cout.setf(ios::right);
cout.fill('0');
cout << setw(2) << date.month << "-" << setw(2) <<
Index: examples/dbinfo.cpp
================================================== =================
RCS file: /usr/local/cvs/mysql++/examples/dbinfo.cpp,v
retrieving revision 1.2
diff -u -r1.2 dbinfo.cpp
--- examples/dbinfo.cpp 1 Mar 2005 03:48:40 -0000 1.2
+++ examples/dbinfo.cpp 5 Apr 2005 17:04:12 -0000
@@ -106,7 +106,7 @@
for (counter = 0; counter < columns; counter++) {
if (widths[counter]) {
cout << ' ' << setw(widths[counter]) <<
- row[counter] << ' ';
+ row.col_num(counter) << ' ';
}
}
cout << endl;
@@ -130,7 +130,7 @@
for (i = res.begin(); i != res.end(); ++i) {
row = *i;
for (int counter = 0; counter < columns; counter++) {
- cout << row[counter] << " ";
+ cout << row.col_num(counter) << " ";
}
cout << endl;
}
Index: examples/util.cpp
================================================== =================
RCS file: /usr/local/cvs/mysql++/examples/util.cpp,v
retrieving revision 1.4
diff -u -r1.4 util.cpp
--- examples/util.cpp 11 Mar 2005 06:08:27 -0000 1.4
+++ examples/util.cpp 5 Apr 2005 17:04:12 -0000
@@ -42,11 +42,11 @@

// Note that you can use either the column index or name to
// retrieve the data.
- cout << setw(20) << row[0].c_str() << ' ' <<
- setw(9) << row[1].c_str() << ' ' <<
- setw(9) << row.lookup_by_name("weight").c_str() << ' ' <<
- setw(9) << row[3].c_str() << ' ' <<
- row[4] << endl;
+ cout << setw(20) << row.col_num(0).c_str() << ' ' <<
+ setw(9) << row.col_num(1).c_str() << ' ' <<
+ setw(9) << row["weight"].c_str() << ' ' <<
+ setw(9) << row.col_num(3).c_str() << ' ' <<
+ row.col_num(4) << endl;
}
}

Index: lib/custom.pl
================================================== =================
RCS file: /usr/local/cvs/mysql++/lib/custom.pl,v
retrieving revision 1.14
diff -u -r1.14 custom.pl
--- lib/custom.pl 14 Feb 2005 17:36:32 -0000 1.14
+++ lib/custom.pl 5 Apr 2005 17:04:12 -0000
@@ -221,7 +221,7 @@
$parm_simple2c_b .= ", " unless $j == $i;
$defs .= " T$j I$j;";
$defs .= "\n" unless $j == $i;
- $popul .= " s->I$j = static_cast<T$j>(row[ O$j ]);";
+ $popul .= " s->I$j = static_cast<T$j>(row.col_num( O$j ));";
$popul .= "\n" unless $j == $i;
$names .= " N$j ";
$names .= ",\n" unless $j == $i;
Index: lib/row.cpp
================================================== =================
RCS file: /usr/local/cvs/mysql++/lib/row.cpp,v
retrieving revision 1.4
diff -u -r1.4 row.cpp
--- lib/row.cpp 1 Mar 2005 03:29:50 -0000 1.4
+++ lib/row.cpp 5 Apr 2005 17:04:12 -0000
@@ -9,7 +9,7 @@
return res->num_fields();
}

-const ColData Row:perator[] (size_type i) const
+const ColData Row::col_num(size_type i) const
{
if (!initialized) {
if (throw_exceptions)
@@ -21,11 +21,11 @@
return ColData(data.at(i).c_str(), res->types(i), is_nulls[i]);
}

-const ColData Row::lookup_by_name(const char* i) const
+const ColData Row:perator[](const char* i) const
{
int si = res->field_num(std::string(i));
if (si < res->num_fields()) {
- return (*this)[si];
+ return col_num(si);
}
else {
throw BadFieldName(i);
Index: lib/row.h
================================================== =================
RCS file: /usr/local/cvs/mysql++/lib/row.h,v
retrieving revision 1.9
diff -u -r1.9 row.h
--- lib/row.h 14 Feb 2005 17:29:54 -0000 1.9
+++ lib/row.h 5 Apr 2005 17:04:12 -0000
@@ -231,8 +231,7 @@
};

//: This class handles the actual rows in an intelligent manner.
-class Row : public const_subscript_container<Row,ColData,const ColData>,
- public RowTemplate<Row, ResUse>
+class Row : public RowTemplate<Row, ResUse>
{
private:
std::vector <std::string> data;
@@ -241,6 +240,8 @@
bool throw_exceptions, initialized;

public:
+ typedef unsigned int size_type;
+
Row() {}
Row(MYSQL_ROW d, const ResUse *r, unsigned long *jj, bool te = false)
: res(r), throw_exceptions(te), initialized(false)
@@ -262,12 +263,12 @@
Row& self() {return *this;}

const ResUse& parent() const {return *res;}
- size_type size() const;
- //: Returns the number of columns.
- const ColData operator [] (size_type i) const;
- //: Returns the value of the field with the index of i.
+ // Returns the number of columns.
+ size_type size() const;
+ // Returns the value of the column named 'col'.
+ const ColData operator [] (const char* col) const;

- const ColData lookup_by_name(const char*) const;
+ const ColData col_num(size_type i) const;

const char *raw_data(int i) const {return data[i].data();}



--------------080408080603010106030602
Content-Type: text/plain; charset=us-ascii

--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw
--------------080408080603010106030602--

Warren Young wrote:
This depends a lot on how important lookup speed is. The integer lookup
is bound to be faster than the string lookup. Though there's already
enough of a speed hit in the mysqlpp::Row stuff that it may not matter.

However, I can say that my code currently does use the numeric index
lookup, because it knows what order the fields are in (they match
another list it's keeping). So there's at least one instance of real
world code that uses it.

--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw

Earl Miles wrote:

Noted, but study the patch a little more carefully: it still provides
Row::col_num() to get a field by column number.

The question ultimately is, which of the two methods of access should be
given the more convenient array index access method?

Code like in lib/custom.h isn't meant to be read very often. Code like
in the MySQL++ examples is definitely intended to be read, often: it
must be as clear as possible. Considering just these two examples, it
would seem logical that it's okay for custom.h to use col_num() lookups
because clarity isn't as valuable as speed here, while the examples
should do lookups by column name, in the interest of maximum code clarity.

--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw

On Tue, Apr 05, 2005 at 11:13:21AM -0600, Warren Young wrote:
I've thought this for a while, since the postgresql library (libpqxx) seems
to make use of operator[](const char[]) const without any problem.

I know that having specific names for things is a benefit in itself
sometimes, so it's not that big a deal to me.


The numeric order is based on the order of the SELECT statement itself,
and as long as the SELECT specifies all the fields it uses, there is
no danger of an order bug popping up due to an underlying table change.

Some of my code does use the numeric indexes, but generally I try to use
lookup_by_name() as much as possible just to avoid boneheaded bugs.



I'm not so keen on removing operator[](size_type), but you have a good
reason with the 0. I should probably run some tests to see how libpqxx
gets around it, or whether they know of the problem at all. Somehow I
wonder how they could get to version 2.3.0 and not have this bother them.

For comparison, here is the code from libpqxx:

util.hxx:
typedef long result_size_type;

result.hxx:
inline field operator[](size_type) const throw ();
field operator[](const char[]) const;
field operator[](const PGSTD::string &s) const
{ return operator[](s.c_str()); }
field at(size_type) const throw (PGSTD:ut_of_range);
field at(const char[]) const;
field at(const PGSTD::string &s) const { return at(s.c_str()); }

I would also recommend we consider the use of "at" instead of "col_num",
to make things more like the std libraries.

- Chris


--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw

Hi,

We reciently upgraded from the older version of mysql++ 1.7.9 (g++ 2.95.4)
to mysql++ 1.7.31 (g++ 3.4.4) and found all our code was broken. After
looking into it we discovered the new lookup_by_name() and found this
thread. I guess we didn't read the release notes that closely :-(

Turns out, we make heavy use of the Row:perator[](const char *). As a
matter of fact, none of the code uses the Row:perator[](int) or
Row:perator[](size_type). We used the Row:perator[](const char *)
because we didn't want to depend on the order of the columns.

We will give your patch a try and let you know how it works.

Bruce



--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw

Chris Frey wrote:
Did you ever find out how libpqxx manages to have operator[] overloaded
for both size_type and const char*?

--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw

--------------090305050000050008050809
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

I just finished re-doing this patch, and it's substantially different
from my previous effort:

- Row's lookup-by-index function is now at(), not col_name(), to work
more like the Standard Library.

- I decided not to remove const_subscript_container as Row's superclass,
but instead change const_subscript_container and all its subclasses
(Row, Fields, and Result) provide at() instead of operator[](int).

- This version is a bit more comprehensive.

- This one applies to the new v2.0 alpha, so it's necessarily somewhat
different than my effort from last April.


So, last time I offered a patch on this subject for comment, the only
objections were that some people were using index-by-int for speed
reasons. I pointed out that you can still use at() (nee' col_num()) for
that.

Could those of you still interested in this try this patch against the
current svn version, and see what you think? Notice how many files this
touches: it's fairly intrusive. That's why I'm running this RFC again,
instead of checking it in.

Personally, I think it's good to have the option of indexing by name,
and find indexing by integer less compelling. If speed is your thing, I
think you should be using SSQLS instead anyway.

--------------090305050000050008050809
Content-Type: text/plain;
name="row_lookup.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="row_lookup.patch"

Index: doc/userman/userman.xml
================================================== =================
--- doc/userman/userman.xml (revision 812)
+++ doc/userman/userman.xml (working copy)
@@ -3006,6 +3006,17 @@
<computeroutput>value_list</computeroutput>,
without any thought of whether it made
sense.</para>
+
+ <para>Row objects can now be indexed by a
+ string again (e.g. row["myfield"]) as in
+ v1.7.9; lookup_by_name() was removed. The
+ underlying problem that lead to that change
+ still exists, so this is at the expense of
+ indexing by an integer. To get a field by
+ its index, use the new at() function, by
+ analogy with vector::at(). This change was
+ done because it's generally more useful to
+ index a Row by a field name.</para>
</sect3>
</sect2>

Index: lib/fields.cpp
================================================== =================
--- lib/fields.cpp (revision 805)
+++ lib/fields.cpp (working copy)
@@ -35,7 +35,7 @@
return res->num_fields();
}

-const Field & Fields:perator[] (Fields::size_type i) const
+const Field& Fields::at(Fields::size_type i) const
{
res->field_seek(i);
return res->fetch_field();
Index: lib/row.h
================================================== =================
--- lib/row.h (revision 808)
+++ lib/row.h (working copy)
@@ -469,11 +469,10 @@
/// \brief Get the number of fields in the row.
size_type size() const;

- /// \brief Get the value of a field given its index.
+ /// \brief Get the value of a field given its name.
///
- /// If the array index is out of bounds, the C++ standard says that
- /// the underlying vector container should throw an exception.
- /// Whether it actually does is probably implementation-dependent.
+ /// If the field does not exist in this row, we throw a BadFieldName
+ /// exception.
///
/// Note that we return the
/// \link mysqlpp::ColData_Tmpl ColData \endlink object by value.
@@ -483,30 +482,33 @@
/// like this:
///
/// \code
- /// string s = row[2];
+ /// string s = row["myfield"];
/// \endcode
///
- /// That accesses the third field in the row, returns a temporary
+ /// That accesses myfield within the row, returns a temporary
/// ColData object, which is then automatically converted to a
/// \c std::string and copied into \c s. That works fine, but
/// beware of this similar but incorrect construct:
///
/// \code
- /// const char* pc = row[2];
+ /// const char* pc = row["myfield"];
/// \endcode
///
/// This one line of code does what you expect, but \c pc is then a
/// dangling pointer: it points to memory owned by the temporary
/// ColData object, which will have been destroyed by the time you
/// get around to actually \e using the pointer.
- const ColData operator [](size_type i) const;
+ ///
+ /// This function is rather inefficient. If that is a concern for
+ /// you, use at() or the SSQLS mechanism instead.
+ const ColData operator [](const char* field) const;

- /// \brief Get the value of a field given its field name.
+ /// \brief Get the value of a field given its index.
///
- /// This function is rather inefficient. You should use operator[]
- /// if you're using Rows directly, or SSQLS for efficient named
- /// access to row elements.
- const ColData lookup_by_name(const char*) const;
+ /// If the index is out-of-bounds, the underlying vector is supposed
+ /// to throw an exception according to the C++ Standard. Whether it
+ /// actually does this is implementation-dependent.
+ const ColData at(size_type i) const;

/// \brief Return the value of a field given its index, in raw form.
///
Index: lib/result.h
================================================== =================
--- lib/result.h (revision 805)
+++ lib/result.h (working copy)
@@ -304,7 +304,7 @@
/// \brief Get the underlying Field structure given its index.
const Field& fields(unsigned int i) const
{
- return fields_[i];
+ return fields_.at(i);
}

/// \brief Returns true if the other ResUse object shares the same
@@ -412,13 +412,15 @@
{
return size_type(num_rows());
}
+
/// \brief Alias for num_rows(), only with different return type.
size_type rows() const
{
return size_type(num_rows());
}
+
/// \brief Get the row with an offset of i.
- const Row operator [](size_type i) const
+ const Row at(size_type i) const
{
data_seek(i);
return fetch_row();
Index: lib/custom.pl
================================================== =================
--- lib/custom.pl (revision 805)
+++ lib/custom.pl (working copy)
@@ -262,7 +262,7 @@
$parm_simple2c_b .= ", " unless $j == $i;
$defs .= " T$j I$j;";
$defs .= "\n" unless $j == $i;
- $popul .= " s->I$j = static_cast<T$j>(row[ O$j ]);";
+ $popul .= " s->I$j = static_cast<T$j>(row.at(O$j));";
$popul .= "\n" unless $j == $i;
$names .= " N$j ";
$names .= ",\n" unless $j == $i;
Index: lib/field_names.cpp
================================================== =================
--- lib/field_names.cpp (revision 805)
+++ lib/field_names.cpp (working copy)
@@ -39,7 +39,7 @@
reserve(num);

for (int i = 0; i < num; i++) {
- std::string p(res->fields()[i].name);
+ std::string p(res->fields().at(i).name);
str_to_lwr(p);
push_back(p);
}
Index: lib/row.cpp
================================================== =================
--- lib/row.cpp (revision 805)
+++ lib/row.cpp (working copy)
@@ -35,7 +35,7 @@
return res_->num_fields();
}

-const ColData Row:perator [](size_type i) const
+const ColData Row::at(size_type i) const
{
if (!initialized_) {
if (throw_exceptions())
@@ -47,14 +47,14 @@
return ColData(data_.at(i).c_str(), res_->types(i), is_nulls_[i]);
}

-const ColData Row::lookup_by_name(const char* i) const
+const ColData Row:perator [](const char* field) const
{
- int si = res_->field_num(std::string(i));
- if (si < res_->num_fields()) {
- return (*this)[si];
+ int si = res_->field_num(std::string(field));
+ if (si < size()) {
+ return at(si);
}
else {
- throw BadFieldName(i);
+ throw BadFieldName(field);
}
}

Index: lib/fields.h
================================================== =================
--- lib/fields.h (revision 805)
+++ lib/fields.h (working copy)
@@ -54,12 +54,12 @@
}

/// \brief Returns a field given its index.
- const Field& operator [](size_type i) const;
+ const Field& at(size_type i) const;

/// \brief Returns a field given its index.
- const Field& operator [](int i) const
+ const Field& at(int i) const
{
- return operator [](size_type(i));
+ return at(static_cast<size_type>(i));
}

size_type size() const; ///< get the number of fields
Index: lib/resiter.h
================================================== =================
--- lib/resiter.h (revision 805)
+++ lib/resiter.h (working copy)
@@ -82,7 +82,7 @@
virtual size_type size() const = 0;

/// \brief Return element at given index in container
- virtual ReturnType operator [](SizeType i) const = 0;
+ virtual ReturnType at(SizeType i) const = 0;

/// \brief Return maximum number of elements that can be stored
/// in container without resizing.
@@ -182,11 +182,11 @@

/// \brief Access the current pointed-to element within the
/// container
- ReturnType* operator ->() const { return &((*d)[i]); }
+ ReturnType* operator ->() const { return &(d->at(i)); }

/// \brief Dereference the iterator, returning the pointed-to
/// element within the container.
- ReturnType operator *() const { return (*d)[i]; }
+ ReturnType operator *() const { return d->at(i); }

/// \brief Return the an element in the container given its index
ReturnType operator [](SizeType n) const { return (*d)[n]; }
Index: Wishlist
================================================== =================
--- Wishlist (revision 808)
+++ Wishlist (working copy)
@@ -60,13 +60,6 @@

o Fold RowTemplate class into Row.

- o Swap the definitions of Row:perator[] and lookup_by_name().
- That is, have the operator take a field name, and define a
- function (at()?) that takes a field index. The original problem
- solved in 1.7.10 still exists, in that modern compilers won't
- let you overload the operator on both strings and integers,
- but the string form probably is the most useful.
-
o Decide on a new soname scheme? It'd be nice if the soname had
some obvious relationship to the library version. We could
actually go with a 1:1 relationship, and simply override the
Index: examples/dbinfo.cpp
================================================== =================
--- examples/dbinfo.cpp (revision 805)
+++ examples/dbinfo.cpp (working copy)
@@ -133,7 +133,7 @@
for (counter = 0; counter < columns; counter++) {
if (widths[counter]) {
cout << ' ' << setw(widths[counter]) <<
- row[counter] << ' ';
+ row.at(counter) << ' ';
}
}
cout << endl;
@@ -157,7 +157,7 @@
for (i = res.begin(); i != res.end(); ++i) {
row = *i;
for (int counter = 0; counter < columns; counter++) {
- cout << row[counter] << " ";
+ cout << row.at(counter) << " ";
}
cout << endl;
}
Index: examples/util.cpp
================================================== =================
--- examples/util.cpp (revision 812)
+++ examples/util.cpp (working copy)
@@ -134,8 +134,15 @@
// specially. (See Row:perator[]'s documentation for the reason.)
// As for the rest of the fields, Row:perator[] returns a ColData
// object, which can convert itself to any standard C type.
- string item(row[0]);
- print_stock_row(item, row[1], row[2], row[3], row[4]);
+ //
+ // We index the row by field name to demonstrate the feature, and
+ // also because it makes the code more robust in the face of schema
+ // changes. Use Row::at() instead if efficiency is paramount. To
+ // maintain efficiency while keeping robustness, use the SSQLS
+ // feature, demoed in the custom* examples.
+ string item(row["item"]);
+ print_stock_row(item, row["num"], row["weight"], row["price"],
+ row["sdate"]);
}


Index: examples/custom3.cpp
================================================== =================
--- examples/custom3.cpp (revision 805)
+++ examples/custom3.cpp (working copy)
@@ -71,7 +71,7 @@
// there's no point in storing the result in an STL container.
// We can store the first row directly into a stock structure
// because one of an SSQLS's constructors takes a Row object.
- stock row = res[0];
+ stock row = res.at(0);

// Create a copy so that the replace query knows what the
// original values are.


--------------090305050000050008050809
Content-Type: text/plain; charset=us-ascii


--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw
--------------090305050000050008050809--

Warren Young wrote:

I guess the thundering silence on this topic means the patch is okay, or
that no one cares enough about it to object. It's applied.

--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsu...ie.nctu.edu.tw