Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

include/denc, kv: silence gcc warnings #13458

Merged
merged 2 commits into from Feb 18, 2017
Merged

include/denc, kv: silence gcc warnings #13458

merged 2 commits into from Feb 18, 2017
+156 −90

Conversation

tchaikov
Copy link
Contributor

No description provided.

adamemerson
adamemerson requested changes Feb 16, 2017
View reviewed changes
src/include/denc.h
@@ -170,9 +170,9 @@ struct denc_traits {
#define WRITE_RAW_DENC(type) \
template<> \
struct denc_traits<type> { \
enum { supported = 2 }; \
enum { featured = false }; \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run this past Sage. From the comments it looks like he did this to prevent some sort of clash with encode/decode, so don't remove it without his imprimatur.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liewegas do we have a test to reproduce the clash? if yes, so does "make check" proves that we are immune from it now? if not, could you enlighten me how to create a reproducer? thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 has a special meaning separate from 1/true

// NOTE: set supported == 2 instead of true. This prevents these from
// getting glued into the legacy encode/decode methods; the overhead
// of setting up a contiguous_appender etc is likely to be slower.

@adamemerson
Copy link
Contributor

Instead of changing the types, how about using enable_if<!!supported ?

@tchaikov
Copy link
Contributor Author

@adamemerson

i am not sure i understand the "clash" here. but i am trying to clarify my understanding. please see § 14.5.6.2 in n3337 [1],

A function template can be overloaded with other function templates and with normal (non-template)
functions. A normal function is not related to a function template (i.e., it is never considered to be a special- ization), even if it has the same name and type as a potentially generated function template specialization.

so the plain (the legacy encode/decode) functions won't clash with the ones defined using the templates or

@liewegas

i think this all about how the overload resolution works in C++.

see § 13.3.3/1 in n3337 [1],

Given these definitions, a viable function F1 is defined to be a better function than another viable function F2 if for all arguments i, ICSi(F1) is not a worse conversion sequence than ICSi(F2), and then ... F1 is a non-template function and F2 is a function template specialization, ...

and i also write a small test to verify this behavior.

could you take a look again? thanks.

[1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf

@tchaikov
include/denc: silence gcc warnings
cdb3ecb 
gcc-7 complains:

ceph/ceph/src/include/denc.h:469:50: warning: enum constant in boolean
context [-Wint-in-bool-context]
 inline typename std::enable_if<traits::supported &&
                                ~~~~~~~~~~~~~~~~~~^~
           traits::featured>::type denc(
           ~~~~~~

so let's use "static constexpr bool" instead of enum.

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov
kv: silence the gcc warning of func foo was hidden
7251ea5 
this silences the warnings like

ceph/ceph/src/kv/KeyValueDB.h:59:18: warning: ‘virtual void
KeyValueDB::TransactionImpl::set(const string&, const char*, size_t,
const bufferlist&)’ was hidden [-Woverloaded-\
virtual]
     virtual void set(
                  ^~~

Signed-off-by: Kefu Chai <kchai@redhat.com>
@adamemerson
Copy link
Contributor

Laminators Glue Things Methodically.

@tchaikov
Copy link
Contributor Author

http://pulpito.ceph.com/kchai-2017-02-18_07:48:13-rados-wip-kefu-testing-2---basic-smithi/

@tchaikov tchaikov merged commit 5e98ce9 into ceph:master Feb 18, 2017
@tchaikov tchaikov deleted the wip-silence-gcc-warnings branch February 18, 2017 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liewegas liewegas liewegas approved these changes

@adamemerson adamemerson adamemerson approved these changes

Assignees
No one assigned
Labels
cleanup needs-qa wip-kefu-testing
Projects
None yet
Milestone
No milestone
3 participants
@tchaikov @adamemerson @liewegas