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.

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

common: Fix clang compilation #13335

Merged
merged 1 commit into from Feb 18, 2017
Merged

Conversation

byo
Copy link

@byo byo commented Feb 9, 2017

Signed-off-by: Bartłomiej Święcki bartlomiej.swiecki@corp.ovh.com

Copy link
Contributor

@wjwithagen wjwithagen left a comment

Choose a reason for hiding this comment

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

@byo
Code now compiles on FreeBSD/Clang

@liuchang0812
Copy link
Contributor

hi, could we merge it now? I need this patch.

PerfHistogram(const PerfHistogram &other)
: m_axes_config(other.m_axes_config) {
PerfHistogram(const PerfHistogram &other) {
for (int i = 0; i < DIM; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could use memcpy() for better performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

@byo on a second thought, we can also leverage std::array for this

diff --git a/src/common/perf_histogram.h b/src/common/perf_histogram.h
index 6d315905db..99274d765d 100644
--- a/src/common/perf_histogram.h
+++ b/src/common/perf_histogram.h
@@ -20,6 +20,7 @@
 #include "include/int_types.h"

 #include <memory>
+#include <array>

 class PerfHistogramCommon {
 public:
@@ -133,7 +134,7 @@ protected:
   std::unique_ptr<atomic64_t[]> m_rawData;

   /// Configuration of axes
-  axis_config_d m_axes_config[DIM];
+  std::array<axis_config_d,DIM> m_axes_config;

   /// Dump histogram counters to a formatter
   void dump_formatted_values(ceph::Formatter *f) const {
@@ -145,8 +146,8 @@ protected:
   /// Get number of all histogram counters
   int64_t get_raw_size() {
     int64_t ret = 1;
-    for (int i = 0; i < DIM; ++i) {
-      ret *= m_axes_config[i].m_buckets;
+    for (const auto &ac : m_axes_config) {
+      ret *= ac.m_buckets;
     }
     return ret;
   }

Copy link
Author

Choose a reason for hiding this comment

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

@tchaikov Great idea! I guess that's what std::array was created for. And I'm almost sure that copying such array to another one would be automatically converted to memcpy if the element type is trivially copyable.

Will give it a try in a second.

@tchaikov
Copy link
Contributor

@liuchang0812 it needs an approval and a qa run.

@liuchang0812
Copy link
Contributor

@tchaikov OK

Signed-off-by: Bartłomiej Święcki <bartlomiej.swiecki@corp.ovh.com>
@byo
Copy link
Author

byo commented Feb 15, 2017

@tchaikov Changed as you suggested, looks much better now, also added small unit test to ensure the copy works correctly.

@byo
Copy link
Author

byo commented Feb 15, 2017

jenkins retest this please

@tchaikov tchaikov self-assigned this Feb 15, 2017
@tchaikov tchaikov merged commit 2a05daa into ceph:master Feb 18, 2017
@byo byo deleted the wip-clang-compilation-fix branch March 3, 2017 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants