scality / Droplet

Cloud storage client library
http://www.scality.com
Other
54 stars 33 forks source link

Upstream some basic fixes. #194

Closed mvwieringen closed 9 years ago

mvwieringen commented 9 years ago

I would like to upstream some basic fixes we did in our fork of droplet. Nothing major just some stuff for using the vfs header in C++ and some other small fixes.

Other stuff is mostly building stuff in OpenBuildService which is probably not to interesting to upstream and some hacked proxy support which we use for talking to RIAK.

Joacchim commented 9 years ago

Hello,

We will be happy to merge this change as soon as possible, but before that I have two small questions about the changes provided:

1 - on commit 2a59de901eed : We would rather keep the enum's typing information instead of creating a purely anonymous enum. For C++ Requirements, I think it would be preferable to have both the typedef and the enum's normal type name. -> typedef enum droplet_option_mask { ... } dpl_option_mask_t; Was there a specific reason you chose the anonymous enum way ?

2 - on commit 5e3026ac6f06 : I do not see where you actually set the default_behavior_flag in the droplet context. How do you plan to set it ? Manually from the calling code ?

mvwieringen commented 9 years ago

The typedef of the enum is something g++ seriously doesn't like e.g. when you typedef the flag field the compiler whines when you try to assign an or-ed value to a variable of that type as it wants only one of the enum values. The idea to use an anonymous enum is that we only need the actual mapping of the name to a value. So you can give the enum a name but the typedef will render is useless again.

Regarding the second question, indeed I just set it in the context didn't bother creating some new function. Think by now that I also don't need it anymore but as it was already coded left it in.

Joacchim commented 9 years ago

Alright, I understand your requirement for the first part. Still, we don't really like the idea of a purely anonymous enum for the C library (although we understand it for the C++ POV), so we would suggest something like this (tweak the second typedef name as you see fit): typedef enum { ... } dpl_option_mask_t; typedef unsigned int dpl_option_mask_cppt; Of course, this second typedef could be part of your client code, but I understand that for lisibility it's better if it's part of the upstream library. Would it suit your needs ?

As for the second part, if it's not used anymore, it might be better not to include it (and avoid possible future confusion). Can you remove that patch form the pull request ?

mvwieringen commented 9 years ago

I don't see a simple way of removing things, so I close this pull request and try a new one.