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/03/23 12:29:51 UTC
[GitHub] [avro] thiru-mg opened a new pull request #1153: C++ Reader and writers modernized
thiru-mg opened a new pull request #1153:
URL: https://github.com/apache/avro/pull/1153
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599754032
##########
File path: lang/c++/api/DataFile.hh
##########
@@ -75,11 +75,11 @@ class AVRO_DECL DataFileWriterBase : boost::noncopyable {
Metadata metadata_;
int64_t lastSync_;
- static std::unique_ptr<OutputStream> makeStream(const char* filename);
+ static std::unique_ptr<OutputStream> makeStream(const char *filename);
Review comment:
From the next installment, I'll precede the actual PR with a format-change 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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599679853
##########
File path: lang/c++/api/Node.hh
##########
@@ -135,8 +135,8 @@ class AVRO_DECL Node : private boost::noncopyable {
doAddLeaf(newLeaf);
}
virtual size_t leaves() const = 0;
- virtual const NodePtr &leafAt(int index) const = 0;
- virtual const GenericDatum &defaultValueAt(int index) {
+ virtual const NodePtr &leafAt(size_t index) const = 0;
Review comment:
did you consider the other change, and using int for `leaves()`?
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599681925
##########
File path: lang/c++/api/NodeImpl.hh
##########
@@ -113,15 +113,15 @@ protected:
return docAttribute_.get();
}
- void doAddLeaf(const NodePtr &newLeaf) override {
+ void doAddLeaf(const NodePtr &newLeaf) final {
leafAttributes_.add(newLeaf);
}
size_t leaves() const override {
return leafAttributes_.size();
}
- const NodePtr &leafAt(int index) const override {
+ const NodePtr &leafAt(size_t index) const override {
Review comment:
it seems you would want to make this final as well? if you are converting the one above?
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599683469
##########
File path: lang/c++/api/Serializer.hh
##########
@@ -71,15 +68,15 @@ class Serializer : private boost::noncopyable
}
void writeBytes(const void *val, size_t size) {
- writer_.writeBytes(val);
+ writer_.writeBytes(val, size);
Review comment:
this seems like it should be tracked in a separate JIRA, it looks like an actual bug fix? Is there an associated unit test?
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599677209
##########
File path: lang/c++/api/Node.hh
##########
@@ -81,14 +81,14 @@ std::ostream &operator<<(std::ostream &os, const Name &n) {
///
/// The Node object uses reference-counted pointers. This is so that schemas
/// may be reused in other schemas, without needing to worry about memory
-/// deallocation for nodes that are added to multiple schema parse trees.
+/// de-allocation for nodes that are added to multiple schema parse trees.
Review comment:
i don't think this change is necessary.
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599691253
##########
File path: lang/c++/impl/DataFile.cc
##########
@@ -306,20 +306,20 @@ void DataFileReaderBase::init(const ValidSchema& readerSchema)
static void drain(InputStream& in)
{
- const uint8_t *p = 0;
+ const uint8_t *p = nullptr;
size_t n = 0;
while (in.next(&p, &n));
}
char hex(unsigned int x)
{
- return x + (x < 10 ? '0' : ('a' - 10));
+ return static_cast<char>(x + (x < 10 ? '0' : ('a' - 10)));
}
std::ostream& operator << (std::ostream& os, const DataFileSync& s)
{
- for (size_t i = 0; i < s.size(); ++i) {
- os << hex(s[i] / 16) << hex(s[i] % 16) << ' ';
+ for (unsigned char i : s) {
Review comment:
it looks like elsewhere uint8_t might be used of rhte same 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] emkornfield commented on a change in pull request #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599685832
##########
File path: lang/c++/api/Stream.hh
##########
@@ -160,23 +160,23 @@ AVRO_DECL OutputStreamPtr memoryOutputStream(size_t chunkSize = 4 * 1024);
* It does not copy the data, the byte array should remain valid
* until the InputStream is used.
*/
-AVRO_DECL InputStreamPtr memoryInputStream(const uint8_t* data, size_t len);
+AVRO_DECL InputStreamPtr memoryInputStream(const uint8_t *data, size_t len);
/**
* Returns a new InputStream with the contents written into an
- * outputstream. The output stream must have been returned by
+ * output stream. The output stream must have been returned by
Review comment:
`OutputStream` seems like the better fix. Rewording this to be Returns a new InputStream that reads the contents written to `source` might be even better.
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599754032
##########
File path: lang/c++/api/DataFile.hh
##########
@@ -75,11 +75,11 @@ class AVRO_DECL DataFileWriterBase : boost::noncopyable {
Metadata metadata_;
int64_t lastSync_;
- static std::unique_ptr<OutputStream> makeStream(const char* filename);
+ static std::unique_ptr<OutputStream> makeStream(const char *filename);
Review comment:
From the next installment, I'll precede the actual PR with a format-change PR. It is a good idea to have a discussion on the style rule. But in order not to get distracted, I'll just stick to what JetBrains CLion does y default (which is what they call Google style)
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599763707
##########
File path: lang/c++/api/Node.hh
##########
@@ -81,14 +81,14 @@ std::ostream &operator<<(std::ostream &os, const Name &n) {
///
/// The Node object uses reference-counted pointers. This is so that schemas
/// may be reused in other schemas, without needing to worry about memory
-/// deallocation for nodes that are added to multiple schema parse trees.
+/// de-allocation for nodes that are added to multiple schema parse trees.
Review comment:
Done
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599780265
##########
File path: lang/c++/impl/DataFile.cc
##########
@@ -243,13 +243,13 @@ void DataFileWriterBase::flush()
sync();
}
-boost::mt19937 random(static_cast<uint32_t>(time(0)));
+boost::mt19937 random(static_cast<uint32_t>(time(nullptr)));
DataFileSync DataFileWriterBase::makeSync()
{
DataFileSync sync;
- for (size_t i = 0; i < sync.size(); ++i) {
- sync[i] = random();
+ for (unsigned char & i : sync) {
Review comment:
Nice. Thanks, done.
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599837039
##########
File path: lang/c++/impl/DataFile.cc
##########
@@ -243,13 +243,13 @@ void DataFileWriterBase::flush()
sync();
}
-boost::mt19937 random(static_cast<uint32_t>(time(0)));
+boost::mt19937 random(static_cast<uint32_t>(time(nullptr)));
DataFileSync DataFileWriterBase::makeSync()
{
DataFileSync sync;
- for (size_t i = 0; i < sync.size(); ++i) {
- sync[i] = random();
+ for (unsigned char & i : sync) {
Review comment:
Nice. Thanks, done.
--
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 pull request #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #1153:
URL: https://github.com/apache/avro/pull/1153#issuecomment-804993202
saw your message on JIRA, will try to take a look tonight
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599767675
##########
File path: lang/c++/api/NodeImpl.hh
##########
@@ -113,15 +113,15 @@ protected:
return docAttribute_.get();
}
- void doAddLeaf(const NodePtr &newLeaf) override {
+ void doAddLeaf(const NodePtr &newLeaf) final {
leafAttributes_.add(newLeaf);
}
size_t leaves() const override {
return leafAttributes_.size();
}
- const NodePtr &leafAt(int index) const override {
+ const NodePtr &leafAt(size_t index) const override {
Review comment:
None of the other virtual methods are called from the constructor and hence leaving them `override` is fine.
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599769653
##########
File path: lang/c++/api/Stream.hh
##########
@@ -160,23 +160,23 @@ AVRO_DECL OutputStreamPtr memoryOutputStream(size_t chunkSize = 4 * 1024);
* It does not copy the data, the byte array should remain valid
* until the InputStream is used.
*/
-AVRO_DECL InputStreamPtr memoryInputStream(const uint8_t* data, size_t len);
+AVRO_DECL InputStreamPtr memoryInputStream(const uint8_t *data, size_t len);
/**
* Returns a new InputStream with the contents written into an
- * outputstream. The output stream must have been returned by
+ * output stream. The output stream must have been returned by
Review comment:
Done
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599681190
##########
File path: lang/c++/api/NodeImpl.hh
##########
@@ -113,15 +113,15 @@ protected:
return docAttribute_.get();
}
- void doAddLeaf(const NodePtr &newLeaf) override {
+ void doAddLeaf(const NodePtr &newLeaf) final {
Review comment:
this is semantic in what looks likes a public header, are you sure noone else has subclassed this class, and might have overridden this method?
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599673828
##########
File path: lang/c++/api/DataFile.hh
##########
@@ -75,11 +75,11 @@ class AVRO_DECL DataFileWriterBase : boost::noncopyable {
Metadata metadata_;
int64_t lastSync_;
- static std::unique_ptr<OutputStream> makeStream(const char* filename);
+ static std::unique_ptr<OutputStream> makeStream(const char *filename);
Review comment:
same comment as on prior PRs we should ensure this type of style change is discussed and agreed upon on the ML. Also, having a GHA with formatter that enforces this one way or another would be nice.
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599676267
##########
File path: lang/c++/api/GenericDatum.hh
##########
@@ -126,44 +128,44 @@ public:
void selectBranch(size_t branch);
/// Makes a new AVRO_NULL datum.
- GenericDatum() : type_(AVRO_NULL), logicalType_(LogicalType::NONE) { }
+ GenericDatum() : type_(AVRO_NULL), logicalType_(LogicalType::NONE) {}
/// Makes a new AVRO_BOOL datum whose value is of type bool.
Review comment:
it might be worth a comment here, about why constructors are not marked 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] thiru-mg commented on a change in pull request #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599766786
##########
File path: lang/c++/api/NodeImpl.hh
##########
@@ -113,15 +113,15 @@ protected:
return docAttribute_.get();
}
- void doAddLeaf(const NodePtr &newLeaf) override {
+ void doAddLeaf(const NodePtr &newLeaf) final {
Review comment:
The reason this is final is because this is being invoked from the constructor. Invoking a non-final virtual function from constructor is confusing at the very least.
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r600269932
##########
File path: lang/c++/api/Serializer.hh
##########
@@ -71,15 +68,15 @@ class Serializer : private boost::noncopyable
}
void writeBytes(const void *val, size_t size) {
- writer_.writeBytes(val);
+ writer_.writeBytes(val, size);
Review comment:
This is indeed a bug. The only caller for `writeBytes` is `serialize` in `AvroSerializer.hh`. `serialize` is not used by any of the code here, but could be used by some of generated code. Without this fix, the code can crash because there is no guarantee that the string is null-terminated.
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599783424
##########
File path: lang/c++/impl/DataFile.cc
##########
@@ -306,20 +306,20 @@ void DataFileReaderBase::init(const ValidSchema& readerSchema)
static void drain(InputStream& in)
{
- const uint8_t *p = 0;
+ const uint8_t *p = nullptr;
size_t n = 0;
while (in.next(&p, &n));
}
char hex(unsigned int x)
{
- return x + (x < 10 ? '0' : ('a' - 10));
+ return static_cast<char>(x + (x < 10 ? '0' : ('a' - 10)));
}
std::ostream& operator << (std::ostream& os, const DataFileSync& s)
{
- for (size_t i = 0; i < s.size(); ++i) {
- os << hex(s[i] / 16) << hex(s[i] % 16) << ' ';
+ for (unsigned char i : s) {
Review comment:
You're right. Fixed
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg merged pull request #1153:
URL: https://github.com/apache/avro/pull/1153
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r600267586
##########
File path: lang/c++/api/Validator.hh
##########
@@ -33,32 +34,32 @@ class AVRO_DECL NullValidator : private boost::noncopyable {
public:
explicit NullValidator(const ValidSchema &schema) {}
- NullValidator() {}
+ NullValidator() = default;
- void setCount(int64_t val) {}
+ void setCount(int64_t) {}
- bool typeIsExpected(Type type) const {
+ static bool typeIsExpected(Type) {
Review comment:
This function is not used and hence it doesn't matter, as of now.
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599687324
##########
File path: lang/c++/api/Validator.hh
##########
@@ -33,32 +34,32 @@ class AVRO_DECL NullValidator : private boost::noncopyable {
public:
explicit NullValidator(const ValidSchema &schema) {}
- NullValidator() {}
+ NullValidator() = default;
- void setCount(int64_t val) {}
+ void setCount(int64_t) {}
- bool typeIsExpected(Type type) const {
+ static bool typeIsExpected(Type) {
Review comment:
this change seems a little strange to me (while probably technically correct)
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599690550
##########
File path: lang/c++/impl/DataFile.cc
##########
@@ -243,13 +243,13 @@ void DataFileWriterBase::flush()
sync();
}
-boost::mt19937 random(static_cast<uint32_t>(time(0)));
+boost::mt19937 random(static_cast<uint32_t>(time(nullptr)));
DataFileSync DataFileWriterBase::makeSync()
{
DataFileSync sync;
- for (size_t i = 0; i < sync.size(); ++i) {
- sync[i] = random();
+ for (unsigned char & i : sync) {
Review comment:
this is less straightforward to me than the previous code. If you want to refactor, I think `std::generate` would work here and be clearer.
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599763598
##########
File path: lang/c++/api/Node.hh
##########
@@ -135,8 +135,8 @@ class AVRO_DECL Node : private boost::noncopyable {
doAddLeaf(newLeaf);
}
virtual size_t leaves() const = 0;
- virtual const NodePtr &leafAt(int index) const = 0;
- virtual const GenericDatum &defaultValueAt(int index) {
+ virtual const NodePtr &leafAt(size_t index) const = 0;
Review comment:
Yes, but this is never going to be negative and size_t is the better alternative.
--
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 #1153: AVRO-3051: C++ Reader and writers modernized
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on a change in pull request #1153:
URL: https://github.com/apache/avro/pull/1153#discussion_r599763824
##########
File path: lang/c++/api/GenericDatum.hh
##########
@@ -126,44 +128,44 @@ public:
void selectBranch(size_t branch);
/// Makes a new AVRO_NULL datum.
- GenericDatum() : type_(AVRO_NULL), logicalType_(LogicalType::NONE) { }
+ GenericDatum() : type_(AVRO_NULL), logicalType_(LogicalType::NONE) {}
/// Makes a new AVRO_BOOL datum whose value is of type bool.
Review comment:
Did that
--
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