common/xmlformatter: turn on underscored and add unittest #12916

liuchang0812 · 2017-01-13T05:55:04Z

As discussed in Dev-maillist, turn on underscored by default.

Please take a look, thanks

tchaikov · 2017-01-13T08:15:45Z

src/common/Formatter.cc

@@ -34,8 +34,8 @@
 #include <boost/format.hpp>


-static char tolower_underscore(const char b) {
-  return ' ' == b ? '_' : std::tolower(b);
+static char underscore(const char b) {


could move this into a private const method of XMLFormatter, so we don't need to duplicate the same pattern everywhere. like

char to_lower_underscore(char c) const { if (c == ' ') { if (m_underscored) { return '_'; } } else if (m_lowercased) { return tolower(c); } return c; }

and do the transform in a single pass

std::transform(e.begin(), e.end(), e.begin(), [this](unsigned char c) { return this->to_lower_underscore(c); });

tchaikov · 2017-01-13T08:17:32Z

src/common/Formatter.h

@@ -173,7 +173,8 @@ namespace ceph {
    std::stringstream m_ss, m_pending_string;
    std::deque<std::string> m_sections;
    bool m_pretty;
-    bool m_lowercased_underscored;
+    bool m_lowercased;
+    bool m_underscored;


could define them as const.

jcsp

Looks good to me

liewegas · 2017-01-13T14:23:48Z

I was actually thinking we should convert all Formatter users to name fields with lowercase and _ (changing the schema) so that XMLFormatter isn't special. It seems like a better convention to follow anyway (although it is a change). What do you all think?

liuchang0812 · 2017-01-13T16:41:57Z

@liewegas I think it's not a perfect choice. There are a lot of uppercase tag name in RadosRGW, Amazon S3 is case sensitive. So it would be better to keep lowercased false and underscored true.

simple example:

HTTP/1.1 200 OK
x-amz-id-2: eftixk72aD6Ap51TnqcoF8eFidJG9Z/2mkiDFu8yU9AS1ed4OpIszj7UDNEHGran
x-amz-request-id: 318BC8BC148832E5
Date: Wed, 28 Oct 2009 22:32:00 GMT
Last-Modified: Sun, 1 Jan 2006 12:00:00 GMT
x-amz-version-id: 3/L4kqtJlcpXroDTDmJ+rmSpXd3dIbrHY+MTRCxf3vjVBH40Nr8X8gdRQBpUMLUo
Content-Length: 124
Content-Type: text/plain
Connection: close
Server: AmazonS3
 
<AccessControlPolicy>
  <Owner>
    <ID>75aa57f09aa0c8caeab4f8c24e99d10f8e7faeebf76c078efc7c6caea54ba06a</ID>
    <DisplayName>mdtd@amazon.com</DisplayName>
  </Owner>
  <AccessControlList>
    <Grant>
      <Grantee xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:type="CanonicalUser">
        <ID>75aa57f09aa0c8caeab4f8c24e99d10f8e7faeebf76c078efc7c6caea54ba06a</ID>
        <DisplayName>mdtd@amazon.com</DisplayName>
      </Grantee>
      <Permission>FULL_CONTROL</Permission>
    </Grant>
  </AccessControlList>
</AccessControlPolicy>

liewegas · 2017-01-13T16:50:10Z

ok!

asdf

robbat2 · 2017-01-15T08:33:51Z

src/common/Formatter.cc

@@ -588,6 +581,17 @@ std::string XMLFormatter::escape_xml_str(const char *str)
  return std::string(&escaped[0]);
 }

+char XMLFormatter::to_lower_underscore(const char c)
+{
+  if (c == ' ') {


Can we improve the readability of this block?

if(m_underscored && c == ' ') { return '_'; else if (m_lowercased) { return std::tolower(c); } return c;

yeah, we could, but we don't need to call std::tolower() if c is " ".

how about

if (m_underscored && c == ' ') { return '_'; } else if (m_lowercased && isupper(c)) { return tolower(c); } return c;

@robbat2 updated! take a look, please

tchaikov · 2017-01-16T01:45:37Z

src/common/Formatter.h

@@ -169,11 +169,13 @@ namespace ceph {
    void print_spaces();
    static std::string escape_xml_str(const char *str);
    void get_attrs_str(const FormatterAttrs *attrs, std::string& attrs_str);
+    char to_lower_underscore(const char c);


could make this a const method.

sure! i would like to update it

it's done, @tchaikov

tchaikov · 2017-01-16T01:56:34Z

liuchang0812 · 2017-01-16T04:06:48Z

I will squash those commits after review. 😄

tchaikov · 2017-01-16T06:55:14Z

src/common/Formatter.cc

      return '_';
-  } else if (m_lowercased) {
+  } else if (m_lowercased && std::isupper(c)) {


IMO, i don't think this improves the readability. it just hurts the performance here. but yeah, probably the performance here is not a bottleneck...

so，need i remove 'isupper' ?

i'd recommend so, if no objections.

std::tolower determines whether c is space or upper actually. I will update it.

liuchang0812 · 2017-01-16T13:30:14Z

@robbat2 @tchaikov Updated!

tchaikov · 2017-01-16T17:08:10Z

src/test/common/test_xmlformatter.cc

+#include "gtest/gtest.h"
+
+#include "common/Formatter.h"
+#include <iostream>


what is this header for?

sorry, used in debug, remove it now.

tchaikov · 2017-01-16T17:10:03Z

src/common/Formatter.cc

+  if (m_underscored && c == ' ') {
+      return '_';
+  } else if (m_lowercased) {
+    // std::tolower will datermine whether c is space or upper.


IMHO, this comment does not help, maybe we can drop it.

tchaikov · 2017-01-16T17:11:17Z

src/test/common/CMakeLists.txt

+    test_xmlformatter.cc
+    )
+add_ceph_unittest(unittest_xmlformatter ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/unittest_xmlformatter)
+target_link_libraries(unittest_xmlformatter global)


might want to link against ceph-common instead. test_xmlformatter.cc is not referencing any symbols from global.

hi, @tchaikov , what's global and diff with ceph-common, i saw that all unittest use global in this CMakeLists.txt. Need we update other test cases?

tchaikov · 2017-01-16T17:11:37Z

src/test/common/test_xmlformatter.cc

+  formatter.dump_string("String", "String");
+  formatter.close_section();
+  formatter.flush(sout);
+  std::string cmp="<xml>\n <Integer>10</Integer>\n <Float>10</Float>\n <String>String</String>\n</xml>\n\n";


nit, spaces around =

tchaikov · 2017-01-16T17:11:45Z

src/test/common/test_xmlformatter.cc

+  formatter.dump_float("Float", 10.0);
+  formatter.dump_string("String", "String");
+  formatter.flush(sout);
+  std::string cmp="<integer>10</integer><float>10</float><string>String</string>";


tchaikov · 2017-01-16T17:12:08Z

src/test/common/test_xmlformatter.cc

+  formatter.dump_float("Float Item", 10.0);
+  formatter.dump_string("String Item", "String");
+  formatter.flush(sout);
+  std::string cmp="<integer_item>10</integer_item>\n<float_item>10</float_item>\n<string_item>String</string_item>\n\n";


ditto. also, could reformat the line like

std::string cmp("<integer_item>10</integer_item>\n" "<float_item>10</float_item>\n" "<string_item>String</string_item>\n\n");

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>

liuchang0812 · 2017-01-18T09:53:39Z

@tchaikov @liewegas I have updated PR. TALP. 😄

tchaikov reviewed Jan 13, 2017

View reviewed changes

tchaikov added the common label Jan 13, 2017

liuchang0812 force-pushed the xmlformatter-fix branch from 02b27a1 to 503e394 Compare January 13, 2017 09:50

jcsp approved these changes Jan 13, 2017

View reviewed changes

tchaikov approved these changes Jan 13, 2017

View reviewed changes

tchaikov added the needs-qa label Jan 13, 2017

tchaikov self-assigned this Jan 13, 2017

liewegas previously approved these changes Jan 13, 2017

View reviewed changes

robbat2 reviewed Jan 15, 2017

View reviewed changes

tchaikov added the wip-kefu-testing label Jan 16, 2017

tchaikov reviewed Jan 16, 2017

View reviewed changes

tchaikov removed the wip-kefu-testing label Jan 16, 2017

tchaikov removed their assignment Jan 16, 2017

tchaikov reviewed Jan 16, 2017

View reviewed changes

liuchang0812 force-pushed the xmlformatter-fix branch from 6268854 to 9923f83 Compare January 16, 2017 13:29

tchaikov reviewed Jan 16, 2017

View reviewed changes

tchaikov requested changes Jan 16, 2017

View reviewed changes

common/xmlformatter: turn on underscored by default, add unittest

a7efe62

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>

liuchang0812 force-pushed the xmlformatter-fix branch from 9923f83 to a7efe62 Compare January 17, 2017 04:02

tchaikov approved these changes Jan 18, 2017

View reviewed changes

tchaikov added the wip-kefu-testing label Jan 22, 2017

tchaikov merged commit 47c59be into ceph:master Jan 24, 2017

liuchang0812 deleted the xmlformatter-fix branch January 24, 2017 16:04

common/xmlformatter: turn on underscored and add unittest #12916

common/xmlformatter: turn on underscored and add unittest #12916

Conversation

liuchang0812 commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcsp left a comment

Choose a reason for hiding this comment

liewegas commented Jan 13, 2017

liuchang0812 commented Jan 13, 2017

liewegas commented Jan 13, 2017 via email

Choose a reason for hiding this comment

tchaikov Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Jan 16, 2017

liuchang0812 commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuchang0812 commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuchang0812 commented Jan 18, 2017

tchaikov Jan 15, 2017 •

edited

Loading