[01/15] kv_flat_btree_async.cc: use vector instead of VLA's
diff mbox

Message ID 1360266123-28972-2-git-send-email-danny.al-gaaf@bisect.de
State New, archived
Headers show

Commit Message

Danny Al-Gaaf Feb. 7, 2013, 7:41 p.m. UTC
Fix "variable length array of non-POD element type" errors caused by
using librados::ObjectWriteOperation VLAs. (-Wvla)

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
---
 src/key_value_store/kv_flat_btree_async.cc | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Sage Weil Feb. 10, 2013, 5:57 a.m. UTC | #1
On Thu, 7 Feb 2013, Danny Al-Gaaf wrote:
> Fix "variable length array of non-POD element type" errors caused by
> using librados::ObjectWriteOperation VLAs. (-Wvla)
> 
> Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
> ---
>  src/key_value_store/kv_flat_btree_async.cc | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/src/key_value_store/kv_flat_btree_async.cc b/src/key_value_store/kv_flat_btree_async.cc
> index 96c6cb0..4342e70 100644
> --- a/src/key_value_store/kv_flat_btree_async.cc
> +++ b/src/key_value_store/kv_flat_btree_async.cc
> @@ -1119,9 +1119,9 @@ int KvFlatBtreeAsync::cleanup(const index_data &idata, const int &errno) {
>      //all changes were created except for updating the index and possibly
>      //deleting the objects. roll forward.
>      vector<pair<pair<int, string>, librados::ObjectWriteOperation*> > ops;
> -    librados::ObjectWriteOperation owos[idata.to_delete.size() + 1];
> +    vector<librados::ObjectWriteOperation*> owos(idata.to_delete.size() + 1);

I haven't read much of the surrounding code, but from what is included 
here I don't think this is equivalent... these are just null pointers 
initially, and so

>      for (int i = 0; i <= (int)idata.to_delete.size(); ++i) {
> -      ops.push_back(make_pair(pair<int, string>(0, ""), &owos[i]));
> +      ops.push_back(make_pair(pair<int, string>(0, ""), owos[i]));

this doesn't do anything useful... owos[i] may as well be NULL.  Why not 
make it

vector<librados::ObjectWriteOperation> owos(...)

?

>      }
>      set_up_ops(vector<object_data>(),
>  	vector<object_data>(), &ops, idata, &err);
> @@ -1883,23 +1883,23 @@ int KvFlatBtreeAsync::set_many(const map<string, bufferlist> &in_map) {
>    to_create[to_create.size() - 1].max_kdata =
>        to_delete[to_delete.size() - 1].max_kdata;
>  
> -  librados::ObjectWriteOperation owos[2 + 2 * to_delete.size()
> -                                      + to_create.size()];
> +  vector<librados::ObjectWriteOperation*> owos(2 + 2 * to_delete.size()
> +                                      + to_create.size());

Same thing here...

>    vector<pair<pair<int, string>, librados::ObjectWriteOperation*> > ops;
>  
>  
>    index_data idata;
> -  set_up_prefix_index(to_create, to_delete, &owos[0], &idata, &err);
> +  set_up_prefix_index(to_create, to_delete, owos[0], &idata, &err);
>  
>    if (verbose) cout << "finished making to_create and to_delete. "
>        << std::endl;
>  
>    ops.push_back(make_pair(
>        pair<int, string>(ADD_PREFIX, index_name),
> -      &owos[0]));
> +      owos[0]));
>    for (int i = 1; i < 2 + 2 * (int)to_delete.size() + (int)to_create.size();
>        i++) {
> -    ops.push_back(make_pair(make_pair(0,""), &owos[i]));
> +    ops.push_back(make_pair(make_pair(0,""), owos[i]));
>    }
>  
>    set_up_ops(to_create, to_delete, &ops, idata, &err);
> -- 
> 1.8.1.2
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Danny Al-Gaaf Feb. 11, 2013, 1:21 p.m. UTC | #2
Am 10.02.2013 06:57, schrieb Sage Weil:
> On Thu, 7 Feb 2013, Danny Al-Gaaf wrote:
>> Fix "variable length array of non-POD element type" errors caused by
>> using librados::ObjectWriteOperation VLAs. (-Wvla)
>>
>> Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
>> ---
>>  src/key_value_store/kv_flat_btree_async.cc | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/key_value_store/kv_flat_btree_async.cc b/src/key_value_store/kv_flat_btree_async.cc
>> index 96c6cb0..4342e70 100644
>> --- a/src/key_value_store/kv_flat_btree_async.cc
>> +++ b/src/key_value_store/kv_flat_btree_async.cc
>> @@ -1119,9 +1119,9 @@ int KvFlatBtreeAsync::cleanup(const index_data &idata, const int &errno) {
>>      //all changes were created except for updating the index and possibly
>>      //deleting the objects. roll forward.
>>      vector<pair<pair<int, string>, librados::ObjectWriteOperation*> > ops;
>> -    librados::ObjectWriteOperation owos[idata.to_delete.size() + 1];
>> +    vector<librados::ObjectWriteOperation*> owos(idata.to_delete.size() + 1);
> 
> I haven't read much of the surrounding code, but from what is included 
> here I don't think this is equivalent... these are just null pointers 
> initially, and so
> 
>>      for (int i = 0; i <= (int)idata.to_delete.size(); ++i) {
>> -      ops.push_back(make_pair(pair<int, string>(0, ""), &owos[i]));
>> +      ops.push_back(make_pair(pair<int, string>(0, ""), owos[i]));
> 
> this doesn't do anything useful... owos[i] may as well be NULL.  Why not 
> make it
> 
> vector<librados::ObjectWriteOperation> owos(...)
> 
> ?

Because this would lead to a linker error:

kv_flat_btree_async.o: In function `void
std::__uninitialized_fill_n<false>::__uninit_fill_n<librados::ObjectWriteOperation*,
unsigned long,
librados::ObjectWriteOperation>(librados::ObjectWriteOperation*,
unsigned long, librados::ObjectWriteOperation const&)':
/usr/bin/../lib64/gcc/x86_64-suse-linux/4.7/../../../../include/c++/4.7/bits/stl_uninitialized.h:188:
undefined reference to
`librados::ObjectOperation::ObjectOperation(librados::ObjectOperation
const&)'
/usr/bin/../lib64/gcc/x86_64-suse-linux/4.7/../../../../include/c++/4.7/bits/stl_uninitialized.h:188:
undefined reference to
`librados::ObjectOperation::ObjectOperation(librados::ObjectOperation
const&)'


Because in src/include/rados/librados.hpp
librados::ObjectOperation::ObjectOperation(librados::ObjectOperation
const&) was is defined, but not implemented in the librados.cc.

Not sure if removing ObjectOperation(librados::ObjectOperation const&)
is the way to go here.

Danny
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sage Weil Feb. 11, 2013, 1:57 p.m. UTC | #3
On Mon, 11 Feb 2013, Danny Al-Gaaf wrote:
> Am 10.02.2013 06:57, schrieb Sage Weil:
> > On Thu, 7 Feb 2013, Danny Al-Gaaf wrote:
> >> Fix "variable length array of non-POD element type" errors caused by
> >> using librados::ObjectWriteOperation VLAs. (-Wvla)
> >>
> >> Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
> >> ---
> >>  src/key_value_store/kv_flat_btree_async.cc | 14 +++++++-------
> >>  1 file changed, 7 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/src/key_value_store/kv_flat_btree_async.cc b/src/key_value_store/kv_flat_btree_async.cc
> >> index 96c6cb0..4342e70 100644
> >> --- a/src/key_value_store/kv_flat_btree_async.cc
> >> +++ b/src/key_value_store/kv_flat_btree_async.cc
> >> @@ -1119,9 +1119,9 @@ int KvFlatBtreeAsync::cleanup(const index_data &idata, const int &errno) {
> >>      //all changes were created except for updating the index and possibly
> >>      //deleting the objects. roll forward.
> >>      vector<pair<pair<int, string>, librados::ObjectWriteOperation*> > ops;
> >> -    librados::ObjectWriteOperation owos[idata.to_delete.size() + 1];
> >> +    vector<librados::ObjectWriteOperation*> owos(idata.to_delete.size() + 1);
> > 
> > I haven't read much of the surrounding code, but from what is included 
> > here I don't think this is equivalent... these are just null pointers 
> > initially, and so
> > 
> >>      for (int i = 0; i <= (int)idata.to_delete.size(); ++i) {
> >> -      ops.push_back(make_pair(pair<int, string>(0, ""), &owos[i]));
> >> +      ops.push_back(make_pair(pair<int, string>(0, ""), owos[i]));
> > 
> > this doesn't do anything useful... owos[i] may as well be NULL.  Why not 
> > make it
> > 
> > vector<librados::ObjectWriteOperation> owos(...)
> > 
> > ?
> 
> Because this would lead to a linker error:
> 
> kv_flat_btree_async.o: In function `void
> std::__uninitialized_fill_n<false>::__uninit_fill_n<librados::ObjectWriteOperation*,
> unsigned long,
> librados::ObjectWriteOperation>(librados::ObjectWriteOperation*,
> unsigned long, librados::ObjectWriteOperation const&)':
> /usr/bin/../lib64/gcc/x86_64-suse-linux/4.7/../../../../include/c++/4.7/bits/stl_uninitialized.h:188:
> undefined reference to
> `librados::ObjectOperation::ObjectOperation(librados::ObjectOperation
> const&)'
> /usr/bin/../lib64/gcc/x86_64-suse-linux/4.7/../../../../include/c++/4.7/bits/stl_uninitialized.h:188:
> undefined reference to
> `librados::ObjectOperation::ObjectOperation(librados::ObjectOperation
> const&)'
> 
> 
> Because in src/include/rados/librados.hpp
> librados::ObjectOperation::ObjectOperation(librados::ObjectOperation
> const&) was is defined, but not implemented in the librados.cc.
> 
> Not sure if removing ObjectOperation(librados::ObjectOperation const&)
> is the way to go here.

Oh, I see... yeah, we shouldn't remove that.  Probably we should 
restructure the code to use a list<>, which doesn't require a copy 
constructor or assignment operator.

Note that this particular code shouldn't hold up the rest of the patches, 
since it's not being used by anything (yet!).

sage
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/src/key_value_store/kv_flat_btree_async.cc b/src/key_value_store/kv_flat_btree_async.cc
index 96c6cb0..4342e70 100644
--- a/src/key_value_store/kv_flat_btree_async.cc
+++ b/src/key_value_store/kv_flat_btree_async.cc
@@ -1119,9 +1119,9 @@  int KvFlatBtreeAsync::cleanup(const index_data &idata, const int &errno) {
     //all changes were created except for updating the index and possibly
     //deleting the objects. roll forward.
     vector<pair<pair<int, string>, librados::ObjectWriteOperation*> > ops;
-    librados::ObjectWriteOperation owos[idata.to_delete.size() + 1];
+    vector<librados::ObjectWriteOperation*> owos(idata.to_delete.size() + 1);
     for (int i = 0; i <= (int)idata.to_delete.size(); ++i) {
-      ops.push_back(make_pair(pair<int, string>(0, ""), &owos[i]));
+      ops.push_back(make_pair(pair<int, string>(0, ""), owos[i]));
     }
     set_up_ops(vector<object_data>(),
 	vector<object_data>(), &ops, idata, &err);
@@ -1883,23 +1883,23 @@  int KvFlatBtreeAsync::set_many(const map<string, bufferlist> &in_map) {
   to_create[to_create.size() - 1].max_kdata =
       to_delete[to_delete.size() - 1].max_kdata;
 
-  librados::ObjectWriteOperation owos[2 + 2 * to_delete.size()
-                                      + to_create.size()];
+  vector<librados::ObjectWriteOperation*> owos(2 + 2 * to_delete.size()
+                                      + to_create.size());
   vector<pair<pair<int, string>, librados::ObjectWriteOperation*> > ops;
 
 
   index_data idata;
-  set_up_prefix_index(to_create, to_delete, &owos[0], &idata, &err);
+  set_up_prefix_index(to_create, to_delete, owos[0], &idata, &err);
 
   if (verbose) cout << "finished making to_create and to_delete. "
       << std::endl;
 
   ops.push_back(make_pair(
       pair<int, string>(ADD_PREFIX, index_name),
-      &owos[0]));
+      owos[0]));
   for (int i = 1; i < 2 + 2 * (int)to_delete.size() + (int)to_create.size();
       i++) {
-    ops.push_back(make_pair(make_pair(0,""), &owos[i]));
+    ops.push_back(make_pair(make_pair(0,""), owos[i]));
   }
 
   set_up_ops(to_create, to_delete, &ops, idata, &err);