Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12849 )
Change subject: KUDU-1711: ColumnSchema supports storing column comment ...................................................................... Patch Set 9: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12849/8/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/12849/8/src/kudu/client/schema.h@288 PS8, Line 288: > I'm sorry, i didn't catch your meaning. I was trying to say two different things. 1) "nullptr" is a reserved word that is only understood by C++ compilers when compiling against the C++11 standard. Kudu as a whole requires a C++11 compliant compiler, but the parts of Kudu exposed in the C++ public API may be built with a compiler that only supports C++03. That's because the public API may be built by system compilers on distros as old as e.g. CentOS 6.6, which ships with gcc 4.4 IIRC (which does not support C++11 at all). Because this header file is part of the public API, you can't use nullptr and have to use NULL; this was already done for the 'default_value' arg in the KuduColumnSchema constructor (see L275). Normally the clang-tidy tool would complain about NULL and ask you to "modernize" it into nullptr; the //NOLINT(modernize-use-nullptr) comment informs clang-tidy of our desire to keep using NULL here. 2) Let's compare the semantics of "const boost::optional<std::string>* comment" and "const std::string* comment". The former allows comment to be in one of four states: - comment is NULL - comment is not NULL and *comment is boost::none - comment is not NULL and *comment is the empty string - comment is not NULL and *comment is a non-empty string We've decided that that both "empty string" and "non-empty string" indicate a valid comment, so this actually boils down to three states: - comment is NULL - comment is not NULL and *comment is boost::none - comment is not NULL and *comment is some string This is one state more than we actually need because both NULL and boost::none can convey the same meaning of "no comment". Meanwhile, "const std::string* comment" allows for only two states: - comment is NULL - comment is not NULL and *comment is some string And these two states can map perfectly to "boost::optional<std::string> comment", which also supports two states: - comment is boost::none - *comment is some string So my point here was that const boost::optional<std::string>* provides more states than we need in order to effectively map to boost::optional<std::string>, and const std::string* comment is enough for what we need. Hope this clarifies what I was trying to explain. -- To view, visit http://gerrit.cloudera.org:8080/12849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82d3228100c262cc4a5c63eaf696927c88bc8990 Gerrit-Change-Number: 12849 Gerrit-PatchSet: 9 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Fri, 29 Mar 2019 04:14:14 +0000 Gerrit-HasComments: Yes
