BewareMyPower commented on code in PR #207:
URL: https://github.com/apache/pulsar-client-cpp/pull/207#discussion_r1150720008
##########
test-conf/standalone-ssl.conf:
##########
@@ -19,6 +19,10 @@
### --- General broker settings --- ###
+# Disable system topic
+systemTopicEnabled=false
+topicLevelPoliciesEnabled=false
Review Comment:
Instead of disabling the system topic, you'd better ignore the system topics
in unit tests (`basicGetNamespaceTopics`) because in future we need to enable
this option for transaction support.
##########
lib/PatternMultiTopicsConsumerImpl.cc:
##########
@@ -193,10 +195,10 @@ void
PatternMultiTopicsConsumerImpl::onTopicsRemoved(NamespaceTopicsPtr removedT
NamespaceTopicsPtr PatternMultiTopicsConsumerImpl::topicsPatternFilter(
const std::vector<std::string>& topics, const
PULSAR_REGEX_NAMESPACE::regex& pattern) {
NamespaceTopicsPtr topicsResultPtr =
std::make_shared<std::vector<std::string>>();
-
- for (std::vector<std::string>::const_iterator itr = topics.begin(); itr !=
topics.end(); itr++) {
- if (PULSAR_REGEX_NAMESPACE::regex_match(*itr, pattern)) {
- topicsResultPtr->push_back(*itr);
+ for (const auto& it : topics) {
Review Comment:
In C++, we usually use `it` or `iter` to represent the iterator. However, in
a range for loop, the `it` here is not an iterator. It's a reference.
```c++
// 1. Iterate
for (auto it = v.cbegin(); it != v.cend(); it++) {
cout << *it << endl; // dereference the iterator to a reference
}
// 2. Range-for loop
for (const auto& value : v) {
cout << value << endl;
}
```
So it's better to name it with `topic` or `topicStr` (to avoid conflict).
##########
lib/HTTPLookupService.h:
##########
@@ -79,7 +79,9 @@ class HTTPLookupService : public LookupService, public
std::enable_shared_from_t
Future<Result, boost::optional<SchemaInfo>> getSchema(const TopicNamePtr&
topicName) override;
- Future<Result, NamespaceTopicsPtr> getTopicsOfNamespaceAsync(const
NamespaceNamePtr& nsName) override;
+ Future<Result, NamespaceTopicsPtr> getTopicsOfNamespaceAsync(
+ const NamespaceNamePtr& nsName,
+ CommandGetTopicsOfNamespace_Mode mode =
CommandGetTopicsOfNamespace_Mode_PERSISTENT) override;
Review Comment:
The default argument in the sub class is meaningless. For example,
```c++
#include <iostream>
#include <memory>
using namespace std;
struct Base {
virtual ~Base() {}
virtual void f(int x = 0) {}
};
struct D1 : Base {
void f(int x) override { cout << "D1::f " << x << endl; }
};
struct D2 : Base {
void f(int x) override { cout << "D2::f " << x << endl; }
};
int main(int argc, char *argv[]) {
std::unique_ptr<Base> pb{new D1};
pb->f();
pb.reset(new D2);
pb->f();
return 0;
}
```
The output is expected:
```
D1::f 0
D2::f 0
```
However, if you changed the `D1` and `D2` implementations to:
```c++
struct D1 : Base {
void f(int x = 1) override { cout << "D1::f " << x << endl; }
};
struct D2 : Base {
void f(int x = 2) override { cout << "D2::f " << x << endl; }
};
```
The output might be unexpected and a little confusing.
```
D1::f 0
D2::f 0
```
Besides, I think we don't need to add the default argument because these
classes are not exposed. The default argument is only good for tests.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
