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.

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

The Clangtastic Mr. Clocks #15186

Merged
merged 2 commits into from May 22, 2017

Conversation

Projects
None yet
3 participants
@adamemerson
Contributor

adamemerson commented May 20, 2017

Unbreak compilation with Clang

@@ -2347,7 +2347,12 @@ int CrushWrapper::_choose_type_stack(
underfull_buckets[j].insert(item);
}
}
ldout(cct, 20) << __func__ << " underfull_buckets " << underfull_buckets << dendl;
ldout(cct, 20) << __func__ << " underfull_buckets " << "[";
for (auto p = underfull_buckets.begin(); p != underfull_buckets.end(); ++p) {

This comment has been minimized.

@joscollin

joscollin May 20, 2017

Member

Please use range-for here.

This comment has been minimized.

@tchaikov

tchaikov May 20, 2017

Contributor

then how can we tell if the current element is the first one?

This comment has been minimized.

@joscollin

joscollin May 20, 2017

Member

Yeah, still we should take the address of each object and compare with begin():

if (&obj != underfull_buckets.begin())

This comment has been minimized.

@tchaikov

tchaikov May 20, 2017

Contributor

i don't think the address of a vector element can be compared with an iterator of that vector. unless you do something like

for (const auto& bucket : underfull_buckets) {
  if (bucket != underfull_buckets.front()) {
 // ...

This comment has been minimized.

@adamemerson

adamemerson May 20, 2017

Contributor

I would normally used ranged for, bit in this case our logic really does depend on the position of the element within the vector rather than just the content of the element. The ways to write that with ranged for are more convoluted than the iterator based version. But, I shall see if Kefu's Forward Declare fix works and if it does do that instead.

This comment has been minimized.

@tchaikov

tchaikov May 20, 2017

Contributor

agreed! should compare the addresses of them if we want to go with the convoluted way.

This comment has been minimized.

@joscollin

joscollin May 20, 2017

Member

if (&obj != underfull_buckets.begin())

aah! sorry my bad. As kefu specified above, it can be compared with the object. But ignore this change as it will make it complicated, not simpler.

if (p != underfull_buckets.begin()) *_dout << ",";
*_dout << *p;
}
*_dout << "]" << dendl;

This comment has been minimized.

@joscollin

joscollin May 20, 2017

Member

From the commit description:

If you are talking about the following << operator function in include/types.h, then I don't think it will work for the below statement, just by looking at the vector<A, Alloc>. In this case, underfull_buckets doesn't match that template. Also it should operate on an ostream object. I think we can overload << for vector<set<int>>, if such instances happen many times, which is the best solution. So please check that.

inline ostream& operator<<(ostream& out, const vector<A,Alloc>& v) {
ldout(cct, 20) << func << " underfull_buckets " << underfull_buckets << dendl;

This comment has been minimized.

@tchaikov

tchaikov May 20, 2017

Contributor

instead of specializing vector<set<int>>, i think we should forward declare

template<class A, class Comp, class Alloc>
inline ostream& operator<<(ostream&, const set<A, Comp, Alloc>&);

before the definition of

template<class A, class Alloc>
inline ostream& operator<<(ostream& out, const vector<A,Alloc>& v)  ///...

then ADL will be able to find the definition operator<<(ostream&, const set<A, Comp, Alloc>&).

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented May 20, 2017

All right! Just forward declared the I/O overloads. One day I'll come over and collapse these into a single template, but that day is not day.

@tchaikov tchaikov added the needs-qa label May 20, 2017

Removed the code that changes were requested on and just forward declared stuff

adamemerson added some commits May 20, 2017

common: Forward-declare container I/O overloads
This will allow strict ADL to find the right overload in the case of a
container of containers and unbreak compilation with Clang.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
rgw: Remove pessimizing move
Do not std::move values from returned functions.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

@adamemerson adamemerson merged commit ad2dbca into ceph:master May 22, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

adamemerson added a commit that referenced this pull request May 22, 2017

Merge pull request #15186 from adamemerson/wip-clangtastic-mr-clocks
rgw: Remove pessimizing move
common: Forward-declare container I/O overloads

Reviewed-by: Kefu Chai <kchai@redhat.com>

@adamemerson adamemerson deleted the adamemerson:wip-clangtastic-mr-clocks branch May 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment