You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2021/02/28 04:46:13 UTC
[GitHub] [avro] thiru-mg opened a new pull request #1106: Core C++ implementation is modernized
thiru-mg opened a new pull request #1106:
URL: https://github.com/apache/avro/pull/1106
Make sure you have checked _all_ steps below.
### Jira
- [ ] My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO/) issues and references them in the PR title. For example, "AVRO-1234: My Avro PR"
- https://issues.apache.org/jira/browse/AVRO-XXX
- In case you are adding a dependency, check if the license complies with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### Commits
- [ ] My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](https://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [ ] In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584427652
##########
File path: lang/c++/impl/ValidSchema.cc
##########
@@ -145,49 +140,49 @@ ValidSchema::toFlatList(std::ostream &os) const
* in UTF-8 format. Note that this method is not responsible for validating
* the schema.
*/
-string ValidSchema::compactSchema(const string& schema) {
- bool insideQuote = false;
- size_t newPos = 0;
- string data(schema.data());
-
- for (size_t currentPos = 0; currentPos < schema.size(); currentPos++) {
- if (!insideQuote && std::isspace(data[currentPos])) {
- // Skip the white spaces outside quotes.
- continue;
- }
+ string ValidSchema::compactSchema(const string &schema) {
+ auto insideQuote = false;
+ auto newPos = 0;
+ auto data = string{schema.data()};
+
+ for (auto currentPos = 0; currentPos < schema.size(); currentPos++) {
Review comment:
fixed in a better way
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584370572
##########
File path: lang/c++/impl/ValidSchema.cc
##########
@@ -145,49 +140,49 @@ ValidSchema::toFlatList(std::ostream &os) const
* in UTF-8 format. Note that this method is not responsible for validating
* the schema.
*/
-string ValidSchema::compactSchema(const string& schema) {
- bool insideQuote = false;
- size_t newPos = 0;
- string data(schema.data());
-
- for (size_t currentPos = 0; currentPos < schema.size(); currentPos++) {
- if (!insideQuote && std::isspace(data[currentPos])) {
- // Skip the white spaces outside quotes.
- continue;
- }
+ string ValidSchema::compactSchema(const string &schema) {
+ auto insideQuote = false;
+ auto newPos = 0;
Review comment:
this seems like a change in semantics? isn't 0 interpreted as integer?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584437294
##########
File path: lang/c++/api/Decoder.hh
##########
@@ -47,11 +47,11 @@ namespace avro {
*/
class AVRO_DECL Decoder {
public:
- virtual ~Decoder() { };
+ virtual ~Decoder() = default;;
/// All future decoding will come from is, which should be valid
/// until replaced by another call to init() or this Decoder is
/// destructed.
- virtual void init(InputStream& is) = 0;
+ virtual void init(InputStream &is) = 0;
Review comment:
This is a highly dogmatic topic. If there isn't a style guide in place for Avro C++ that addresses this already, I think it is worth at least a heads up and separating it into a different PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584370444
##########
File path: lang/c++/impl/ValidSchema.cc
##########
@@ -145,49 +140,49 @@ ValidSchema::toFlatList(std::ostream &os) const
* in UTF-8 format. Note that this method is not responsible for validating
* the schema.
*/
-string ValidSchema::compactSchema(const string& schema) {
- bool insideQuote = false;
- size_t newPos = 0;
- string data(schema.data());
-
- for (size_t currentPos = 0; currentPos < schema.size(); currentPos++) {
- if (!insideQuote && std::isspace(data[currentPos])) {
- // Skip the white spaces outside quotes.
- continue;
- }
+ string ValidSchema::compactSchema(const string &schema) {
+ auto insideQuote = false;
Review comment:
changes like this to auto don't really seem to add value?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r586170698
##########
File path: lang/c++/api/NodeConcepts.hh
##########
@@ -47,33 +47,29 @@ namespace avro {
namespace concepts {
-template <typename Attribute>
-struct NoAttribute
-{
+template<typename Attribute>
+struct NoAttribute {
static const bool hasAttribute = false;
size_t size() const {
return 0;
}
- void add( const Attribute &attr) {
+ void add(const Attribute &attr) {
Review comment:
Got it and fixed it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584420931
##########
File path: lang/c++/api/Decoder.hh
##########
@@ -47,11 +47,11 @@ namespace avro {
*/
class AVRO_DECL Decoder {
public:
- virtual ~Decoder() { };
+ virtual ~Decoder() = default;;
/// All future decoding will come from is, which should be valid
/// until replaced by another call to init() or this Decoder is
/// destructed.
- virtual void init(InputStream& is) = 0;
+ virtual void init(InputStream &is) = 0;
Review comment:
This change makes sense because
int& i, j;
is actually
int &i, j
That is `i` is a reference while `j` is a value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584419892
##########
File path: lang/c++/api/NodeConcepts.hh
##########
@@ -47,33 +47,29 @@ namespace avro {
namespace concepts {
-template <typename Attribute>
-struct NoAttribute
-{
+template<typename Attribute>
+struct NoAttribute {
static const bool hasAttribute = false;
size_t size() const {
return 0;
}
- void add( const Attribute &attr) {
+ void add(const Attribute &attr) {
// There must be an add function for the generic NodeImpl, but the
// Node APIs ensure that it is never called, the throw here is
// just in case
throw Exception("This type does not have attribute");
}
- const Attribute &get(size_t index = 0) const {
+ const Attribute &get(size_t = 0) const {
Review comment:
Good point. Will fix it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584373442
##########
File path: lang/c++/api/NodeConcepts.hh
##########
@@ -169,51 +157,47 @@ struct MultiAttribute
return attrs_.at(index);
}
- private:
+private:
std::vector<Attribute> attrs_;
};
-
template<typename T>
struct NameIndexConcept {
bool lookup(const std::string &name, size_t &index) const {
throw Exception("Name index does not exist");
- return 0;
}
- bool add(const::std::string &name, size_t index) {
+ bool add(const ::std::string &name, size_t) {
Review comment:
`add(const ::std::string &name, size_t /*index*/)` seems more readable. It also appears name is unused.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584369343
##########
File path: lang/c++/api/Compiler.hh
##########
@@ -46,17 +46,17 @@ AVRO_DECL void compileJsonSchema(std::istream &is, ValidSchema &schema);
///
AVRO_DECL bool compileJsonSchema(std::istream &is, ValidSchema &schema,
- std::string &error);
+ std::string &error);
-AVRO_DECL ValidSchema compileJsonSchemaFromStream(InputStream& is);
+AVRO_DECL ValidSchema compileJsonSchemaFromStream(InputStream &is);
Review comment:
Is this reformat inline with the style guide? is there one for the C++ implementation. At least in this file it seems the outlier is the one on line 48.
##########
File path: lang/c++/api/Decoder.hh
##########
@@ -47,11 +47,11 @@ namespace avro {
*/
class AVRO_DECL Decoder {
public:
- virtual ~Decoder() { };
+ virtual ~Decoder() = default;;
/// All future decoding will come from is, which should be valid
/// until replaced by another call to init() or this Decoder is
/// destructed.
- virtual void init(InputStream& is) = 0;
+ virtual void init(InputStream &is) = 0;
Review comment:
based on all the reformatting, it seems like the more accepted version is to put the reference with the type?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584419806
##########
File path: lang/c++/api/Schema.hh
##########
@@ -51,95 +51,94 @@ public:
return node_;
}
- protected:
- Schema();
- explicit Schema(const NodePtr &node);
- explicit Schema(Node *node);
+ protected:
+ explicit Schema(NodePtr node) : node_(std::move(node)) {}
+ explicit Schema(Node *node) : node_(node) {}
NodePtr node_;
};
class AVRO_DECL NullSchema : public Schema {
-public:
- NullSchema(): Schema(new NodePrimitive(AVRO_NULL)) {}
+ public:
+ NullSchema() : Schema(new NodePrimitive(AVRO_NULL)) {}
};
class AVRO_DECL BoolSchema : public Schema {
-public:
- BoolSchema(): Schema(new NodePrimitive(AVRO_BOOL)) {}
+ public:
+ BoolSchema() : Schema(new NodePrimitive(AVRO_BOOL)) {}
};
class AVRO_DECL IntSchema : public Schema {
-public:
- IntSchema(): Schema(new NodePrimitive(AVRO_INT)) {}
+ public:
+ IntSchema() : Schema(new NodePrimitive(AVRO_INT)) {}
};
class AVRO_DECL LongSchema : public Schema {
-public:
- LongSchema(): Schema(new NodePrimitive(AVRO_LONG)) {}
+ public:
+ LongSchema() : Schema(new NodePrimitive(AVRO_LONG)) {}
};
class AVRO_DECL FloatSchema : public Schema {
-public:
- FloatSchema(): Schema(new NodePrimitive(AVRO_FLOAT)) {}
+ public:
+ FloatSchema() : Schema(new NodePrimitive(AVRO_FLOAT)) {}
};
class AVRO_DECL DoubleSchema : public Schema {
-public:
- DoubleSchema(): Schema(new NodePrimitive(AVRO_DOUBLE)) {}
+ public:
+ DoubleSchema() : Schema(new NodePrimitive(AVRO_DOUBLE)) {}
};
class AVRO_DECL StringSchema : public Schema {
-public:
- StringSchema(): Schema(new NodePrimitive(AVRO_STRING)) {}
+ public:
+ StringSchema() : Schema(new NodePrimitive(AVRO_STRING)) {}
};
class AVRO_DECL BytesSchema : public Schema {
-public:
- BytesSchema(): Schema(new NodePrimitive(AVRO_BYTES)) {}
+ public:
+ BytesSchema() : Schema(new NodePrimitive(AVRO_BYTES)) {}
};
class AVRO_DECL RecordSchema : public Schema {
-public:
- RecordSchema(const std::string &name);
+ public:
+ explicit RecordSchema(const std::string &name);
Review comment:
You are right, but very unlikely. I try to avoid explicit, where automatic conversion is handy. E.g conversion of different types to GenericDatum. I don't think anyone would have automatically converted a string into RecordSchema. If they have done, it makes sense for them to make their intention explicit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584371709
##########
File path: lang/c++/api/Encoder.hh
##########
@@ -51,11 +51,11 @@ namespace avro {
*/
class AVRO_DECL Encoder {
public:
- virtual ~Encoder() { };
+ virtual ~Encoder() = default;;
Review comment:
double semicolon?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584373150
##########
File path: lang/c++/api/NodeConcepts.hh
##########
@@ -47,33 +47,29 @@ namespace avro {
namespace concepts {
-template <typename Attribute>
-struct NoAttribute
-{
+template<typename Attribute>
+struct NoAttribute {
static const bool hasAttribute = false;
size_t size() const {
return 0;
}
- void add( const Attribute &attr) {
+ void add(const Attribute &attr) {
// There must be an add function for the generic NodeImpl, but the
// Node APIs ensure that it is never called, the throw here is
// just in case
throw Exception("This type does not have attribute");
}
- const Attribute &get(size_t index = 0) const {
+ const Attribute &get(size_t = 0) const {
Review comment:
it would probaby be more readable to do something like `get(size_t /*index*/ = 0`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584433746
##########
File path: lang/c++/api/NodeConcepts.hh
##########
@@ -47,33 +47,29 @@ namespace avro {
namespace concepts {
-template <typename Attribute>
-struct NoAttribute
-{
+template<typename Attribute>
+struct NoAttribute {
static const bool hasAttribute = false;
size_t size() const {
return 0;
}
- void add( const Attribute &attr) {
+ void add(const Attribute &attr) {
Review comment:
This is because `NoAttribute` is passed to some template which would call `add` on it conditionally. The condition is never met for this specific type and hence this exception is never seen. But the compiler would complain without this function. Not a great design, but we have it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584431937
##########
File path: lang/c++/api/Types.hh
##########
@@ -69,34 +69,34 @@ inline bool isPrimitive(Type t) {
* Primitive types are: string, bytes, int, long, float, double, boolean
* and null
*/
-inline bool isCompound(Type t) {
+inline bool isCompound(Type t) noexcept {
return (t>= AVRO_RECORD) && (t < AVRO_NUM_TYPES);
}
/**
* Returns true if and only if the given type is a valid avro type.
*/
-inline bool isAvroType(Type t) {
+inline bool isAvroType(Type t) noexcept {
return (t >= AVRO_STRING) && (t < AVRO_NUM_TYPES);
}
/**
* Returns true if and only if the given type is within the valid range
* of enumeration.
*/
-inline bool isAvroTypeOrPseudoType(Type t) {
+inline bool isAvroTypeOrPseudoType(Type t) noexcept {
return (t >= AVRO_STRING) && (t <= AVRO_NUM_TYPES);
}
/**
* Converts the given type into a string. Useful for generating messages.
*/
-AVRO_DECL const std::string& toString(Type type);
+AVRO_DECL const std::string& toString(Type type) noexcept;
/**
* Writes a string form of the given type into the given ostream.
*/
-AVRO_DECL std::ostream &operator<< (std::ostream &os, avro::Type type);
+AVRO_DECL std::ostream &operator<< (std::ostream &os, avro::Type type) noexcept;
Review comment:
Removed noexcept
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584438333
##########
File path: lang/c++/api/NodeConcepts.hh
##########
@@ -47,33 +47,29 @@ namespace avro {
namespace concepts {
-template <typename Attribute>
-struct NoAttribute
-{
+template<typename Attribute>
+struct NoAttribute {
static const bool hasAttribute = false;
size_t size() const {
return 0;
}
- void add( const Attribute &attr) {
+ void add(const Attribute &attr) {
Review comment:
I might be misunderstanding (or maybe my comment wasn't clear). But it seems like `void add(const Attribute& /*attr*/)` would still compile?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584372070
##########
File path: lang/c++/api/Types.hh
##########
@@ -60,7 +60,7 @@ enum Type {
* Primitive types are: string, bytes, int, long, float, double, boolean
* and null
*/
-inline bool isPrimitive(Type t) {
+inline bool isPrimitive(Type t) noexcept {
Review comment:
can some of these be constexpr?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg merged pull request #1106: AVRO-3051: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg merged pull request #1106:
URL: https://github.com/apache/avro/pull/1106
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584373580
##########
File path: lang/c++/api/NodeConcepts.hh
##########
@@ -47,33 +47,29 @@ namespace avro {
namespace concepts {
-template <typename Attribute>
-struct NoAttribute
-{
+template<typename Attribute>
+struct NoAttribute {
static const bool hasAttribute = false;
size_t size() const {
return 0;
}
- void add( const Attribute &attr) {
+ void add(const Attribute &attr) {
Review comment:
why not remove attr if it isn't used here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584418814
##########
File path: lang/c++/impl/ValidSchema.cc
##########
@@ -145,49 +140,49 @@ ValidSchema::toFlatList(std::ostream &os) const
* in UTF-8 format. Note that this method is not responsible for validating
* the schema.
*/
-string ValidSchema::compactSchema(const string& schema) {
- bool insideQuote = false;
- size_t newPos = 0;
- string data(schema.data());
-
- for (size_t currentPos = 0; currentPos < schema.size(); currentPos++) {
- if (!insideQuote && std::isspace(data[currentPos])) {
- // Skip the white spaces outside quotes.
- continue;
- }
+ string ValidSchema::compactSchema(const string &schema) {
+ auto insideQuote = false;
Review comment:
Scott Mayer's advice (in Effective Modern C++) is to use `auto` unless, the default detection is something you don't want.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584423849
##########
File path: lang/c++/impl/ValidSchema.cc
##########
@@ -145,49 +140,49 @@ ValidSchema::toFlatList(std::ostream &os) const
* in UTF-8 format. Note that this method is not responsible for validating
* the schema.
*/
-string ValidSchema::compactSchema(const string& schema) {
- bool insideQuote = false;
- size_t newPos = 0;
- string data(schema.data());
-
- for (size_t currentPos = 0; currentPos < schema.size(); currentPos++) {
- if (!insideQuote && std::isspace(data[currentPos])) {
- // Skip the white spaces outside quotes.
- continue;
- }
+ string ValidSchema::compactSchema(const string &schema) {
+ auto insideQuote = false;
+ auto newPos = 0;
+ auto data = string{schema.data()};
Review comment:
Yes. Changing it back.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584437813
##########
File path: lang/c++/impl/ValidSchema.cc
##########
@@ -145,49 +140,49 @@ ValidSchema::toFlatList(std::ostream &os) const
* in UTF-8 format. Note that this method is not responsible for validating
* the schema.
*/
-string ValidSchema::compactSchema(const string& schema) {
- bool insideQuote = false;
- size_t newPos = 0;
- string data(schema.data());
-
- for (size_t currentPos = 0; currentPos < schema.size(); currentPos++) {
- if (!insideQuote && std::isspace(data[currentPos])) {
- // Skip the white spaces outside quotes.
- continue;
- }
+ string ValidSchema::compactSchema(const string &schema) {
+ auto insideQuote = false;
Review comment:
That is fair. I'm not sure I buy all of his arguments of Meyer's, I think it is highly dependent assumed tooling.. I think when refactoring it doesn't really provide value to replace the types, and can introduce bugs (as noted on the integer narrowing). It seems like a good practice for new code?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584373484
##########
File path: lang/c++/api/NodeConcepts.hh
##########
@@ -169,51 +157,47 @@ struct MultiAttribute
return attrs_.at(index);
}
- private:
+private:
std::vector<Attribute> attrs_;
};
-
template<typename T>
struct NameIndexConcept {
bool lookup(const std::string &name, size_t &index) const {
Review comment:
why is the signature not changed here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584372679
##########
File path: lang/c++/api/Schema.hh
##########
@@ -51,95 +51,94 @@ public:
return node_;
}
- protected:
- Schema();
- explicit Schema(const NodePtr &node);
- explicit Schema(Node *node);
+ protected:
+ explicit Schema(NodePtr node) : node_(std::move(node)) {}
+ explicit Schema(Node *node) : node_(node) {}
NodePtr node_;
};
class AVRO_DECL NullSchema : public Schema {
-public:
- NullSchema(): Schema(new NodePrimitive(AVRO_NULL)) {}
+ public:
+ NullSchema() : Schema(new NodePrimitive(AVRO_NULL)) {}
};
class AVRO_DECL BoolSchema : public Schema {
-public:
- BoolSchema(): Schema(new NodePrimitive(AVRO_BOOL)) {}
+ public:
+ BoolSchema() : Schema(new NodePrimitive(AVRO_BOOL)) {}
};
class AVRO_DECL IntSchema : public Schema {
-public:
- IntSchema(): Schema(new NodePrimitive(AVRO_INT)) {}
+ public:
+ IntSchema() : Schema(new NodePrimitive(AVRO_INT)) {}
};
class AVRO_DECL LongSchema : public Schema {
-public:
- LongSchema(): Schema(new NodePrimitive(AVRO_LONG)) {}
+ public:
+ LongSchema() : Schema(new NodePrimitive(AVRO_LONG)) {}
};
class AVRO_DECL FloatSchema : public Schema {
-public:
- FloatSchema(): Schema(new NodePrimitive(AVRO_FLOAT)) {}
+ public:
+ FloatSchema() : Schema(new NodePrimitive(AVRO_FLOAT)) {}
};
class AVRO_DECL DoubleSchema : public Schema {
-public:
- DoubleSchema(): Schema(new NodePrimitive(AVRO_DOUBLE)) {}
+ public:
+ DoubleSchema() : Schema(new NodePrimitive(AVRO_DOUBLE)) {}
};
class AVRO_DECL StringSchema : public Schema {
-public:
- StringSchema(): Schema(new NodePrimitive(AVRO_STRING)) {}
+ public:
+ StringSchema() : Schema(new NodePrimitive(AVRO_STRING)) {}
};
class AVRO_DECL BytesSchema : public Schema {
-public:
- BytesSchema(): Schema(new NodePrimitive(AVRO_BYTES)) {}
+ public:
+ BytesSchema() : Schema(new NodePrimitive(AVRO_BYTES)) {}
};
class AVRO_DECL RecordSchema : public Schema {
-public:
- RecordSchema(const std::string &name);
+ public:
+ explicit RecordSchema(const std::string &name);
Review comment:
This is probbably a good change but this does potentially break source compatibility
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584419005
##########
File path: lang/c++/api/Types.hh
##########
@@ -60,7 +60,7 @@ enum Type {
* Primitive types are: string, bytes, int, long, float, double, boolean
* and null
*/
-inline bool isPrimitive(Type t) {
+inline bool isPrimitive(Type t) noexcept {
Review comment:
Yes. Will fix it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584419941
##########
File path: lang/c++/api/NodeConcepts.hh
##########
@@ -169,51 +157,47 @@ struct MultiAttribute
return attrs_.at(index);
}
- private:
+private:
std::vector<Attribute> attrs_;
};
-
template<typename T>
struct NameIndexConcept {
bool lookup(const std::string &name, size_t &index) const {
throw Exception("Name index does not exist");
- return 0;
}
- bool add(const::std::string &name, size_t index) {
+ bool add(const ::std::string &name, size_t) {
Review comment:
Will fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584370507
##########
File path: lang/c++/impl/ValidSchema.cc
##########
@@ -145,49 +140,49 @@ ValidSchema::toFlatList(std::ostream &os) const
* in UTF-8 format. Note that this method is not responsible for validating
* the schema.
*/
-string ValidSchema::compactSchema(const string& schema) {
- bool insideQuote = false;
- size_t newPos = 0;
- string data(schema.data());
-
- for (size_t currentPos = 0; currentPos < schema.size(); currentPos++) {
- if (!insideQuote && std::isspace(data[currentPos])) {
- // Skip the white spaces outside quotes.
- continue;
- }
+ string ValidSchema::compactSchema(const string &schema) {
+ auto insideQuote = false;
+ auto newPos = 0;
+ auto data = string{schema.data()};
Review comment:
the prior line of code actually appears more idiomatic to me?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584439197
##########
File path: lang/c++/api/Types.hh
##########
@@ -106,7 +106,7 @@ struct AVRO_DECL Null { };
* \param os The ostream to write to.
* \param null The value to be written.
*/
-std::ostream& operator<< (std::ostream &os, const Null &null);
+std::ostream& operator<< (std::ostream &os, const Null &null) noexcept;
Review comment:
same noexcept issue here I think.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584437918
##########
File path: lang/c++/impl/ValidSchema.cc
##########
@@ -145,49 +140,49 @@ ValidSchema::toFlatList(std::ostream &os) const
* in UTF-8 format. Note that this method is not responsible for validating
* the schema.
*/
-string ValidSchema::compactSchema(const string& schema) {
- bool insideQuote = false;
- size_t newPos = 0;
- string data(schema.data());
-
- for (size_t currentPos = 0; currentPos < schema.size(); currentPos++) {
- if (!insideQuote && std::isspace(data[currentPos])) {
- // Skip the white spaces outside quotes.
- continue;
- }
+ string ValidSchema::compactSchema(const string &schema) {
+ auto insideQuote = false;
+ auto newPos = 0;
+ auto data = string{schema.data()};
Review comment:
`string{schema.data};` seems to be what Meyer's would advocate more but still seems like extra typing to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584420931
##########
File path: lang/c++/api/Decoder.hh
##########
@@ -47,11 +47,11 @@ namespace avro {
*/
class AVRO_DECL Decoder {
public:
- virtual ~Decoder() { };
+ virtual ~Decoder() = default;;
/// All future decoding will come from is, which should be valid
/// until replaced by another call to init() or this Decoder is
/// destructed.
- virtual void init(InputStream& is) = 0;
+ virtual void init(InputStream &is) = 0;
Review comment:
This change makes sense because
int& i, j;
is actually
int &i, j
##########
File path: lang/c++/api/Decoder.hh
##########
@@ -47,11 +47,11 @@ namespace avro {
*/
class AVRO_DECL Decoder {
public:
- virtual ~Decoder() { };
+ virtual ~Decoder() = default;;
/// All future decoding will come from is, which should be valid
/// until replaced by another call to init() or this Decoder is
/// destructed.
- virtual void init(InputStream& is) = 0;
+ virtual void init(InputStream &is) = 0;
Review comment:
This change makes sense because
int& i, j;
is actually
int &i, j
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584369872
##########
File path: lang/c++/api/Types.hh
##########
@@ -69,34 +69,34 @@ inline bool isPrimitive(Type t) {
* Primitive types are: string, bytes, int, long, float, double, boolean
* and null
*/
-inline bool isCompound(Type t) {
+inline bool isCompound(Type t) noexcept {
return (t>= AVRO_RECORD) && (t < AVRO_NUM_TYPES);
}
/**
* Returns true if and only if the given type is a valid avro type.
*/
-inline bool isAvroType(Type t) {
+inline bool isAvroType(Type t) noexcept {
return (t >= AVRO_STRING) && (t < AVRO_NUM_TYPES);
}
/**
* Returns true if and only if the given type is within the valid range
* of enumeration.
*/
-inline bool isAvroTypeOrPseudoType(Type t) {
+inline bool isAvroTypeOrPseudoType(Type t) noexcept {
return (t >= AVRO_STRING) && (t <= AVRO_NUM_TYPES);
}
/**
* Converts the given type into a string. Useful for generating messages.
*/
-AVRO_DECL const std::string& toString(Type type);
+AVRO_DECL const std::string& toString(Type type) noexcept;
/**
* Writes a string form of the given type into the given ostream.
*/
-AVRO_DECL std::ostream &operator<< (std::ostream &os, avro::Type type);
+AVRO_DECL std::ostream &operator<< (std::ostream &os, avro::Type type) noexcept;
Review comment:
can't the underlying ostream throw? Is this guaranteed to be noexcept?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584372070
##########
File path: lang/c++/api/Types.hh
##########
@@ -60,7 +60,7 @@ enum Type {
* Primitive types are: string, bytes, int, long, float, double, boolean
* and null
*/
-inline bool isPrimitive(Type t) {
+inline bool isPrimitive(Type t) noexcept {
Review comment:
can these be constexpr?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] emkornfield commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584370871
##########
File path: lang/c++/impl/ValidSchema.cc
##########
@@ -145,49 +140,49 @@ ValidSchema::toFlatList(std::ostream &os) const
* in UTF-8 format. Note that this method is not responsible for validating
* the schema.
*/
-string ValidSchema::compactSchema(const string& schema) {
- bool insideQuote = false;
- size_t newPos = 0;
- string data(schema.data());
-
- for (size_t currentPos = 0; currentPos < schema.size(); currentPos++) {
- if (!insideQuote && std::isspace(data[currentPos])) {
- // Skip the white spaces outside quotes.
- continue;
- }
+ string ValidSchema::compactSchema(const string &schema) {
+ auto insideQuote = false;
+ auto newPos = 0;
+ auto data = string{schema.data()};
+
+ for (auto currentPos = 0; currentPos < schema.size(); currentPos++) {
Review comment:
isn't this also a change in semantics? (size_to to int)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a change in pull request #1106: Core C++ implementation is modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1106:
URL: https://github.com/apache/avro/pull/1106#discussion_r584426328
##########
File path: lang/c++/impl/ValidSchema.cc
##########
@@ -145,49 +140,49 @@ ValidSchema::toFlatList(std::ostream &os) const
* in UTF-8 format. Note that this method is not responsible for validating
* the schema.
*/
-string ValidSchema::compactSchema(const string& schema) {
- bool insideQuote = false;
- size_t newPos = 0;
- string data(schema.data());
-
- for (size_t currentPos = 0; currentPos < schema.size(); currentPos++) {
- if (!insideQuote && std::isspace(data[currentPos])) {
- // Skip the white spaces outside quotes.
- continue;
- }
+ string ValidSchema::compactSchema(const string &schema) {
+ auto insideQuote = false;
+ auto newPos = 0;
Review comment:
Changed it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org