issue #21 #28
issue #21 #28
Conversation
ndn-svs/common.hpp
Outdated
| #include <ndn-cxx/face.hpp> | ||
| #include <ndn-cxx/interest.hpp> | ||
| #include <ndn-cxx/security/validator.hpp> | ||
| #include <ndn-cxx/util/regex.hpp> |
There was a problem hiding this comment.
please include this in the files where it's actually needed, not in common.hpp
There was a problem hiding this comment.
Thank you for your review and I will update them accordingly
ndn-svs/svspubsub.hpp
Outdated
| SubscriptionCallback callback; | ||
| bool isPacketSubscription; | ||
| bool prefetch; | ||
| std::shared_ptr<Regex> regex = make_shared<Regex>("^<>+$"); |
ndn-svs/svspubsub.cpp
Outdated
| NodeID nid = nodePrefix == EMPTY_NAME ? m_dataPrefix : nodePrefix; | ||
| SeqNo seqNo = m_svsync.getCore().getSeqNo(nid) + 1; | ||
|
|
||
| // Insert mapping and manually update the sequence number | ||
| insertMapping(nid, seqNo, name, mappingBlocks); | ||
| m_svsync.getCore().updateSeqNo(seqNo, nid); | ||
|
|
||
| return seqNo; |
ndn-svs/svspubsub.cpp
Outdated
| for (const auto& sub : m_prefixSubscriptions) | ||
| { | ||
| if (sub.prefix.isPrefixOf(mapping.first)) | ||
| if (sub.prefix.isPrefixOf(mapping.first) and sub.autofetch) |
ndn-svs/svspubsub.cpp
Outdated
| m_fetchMap[std::pair(nodeId, seqNo)].push_back(sub); | ||
| queued = true; | ||
| } | ||
| else if (sub.prefix.isPrefixOf(mapping.first) and !sub.autofetch) |
There was a problem hiding this comment.
refactor the if/else to avoid repeating the conditions
ndn-svs/svspubsub.cpp
Outdated
| ndn::span<const uint8_t>{}, | ||
| nodeId, | ||
| seqNo, | ||
| ndn::Data() |
There was a problem hiding this comment.
I don't love this "passing an empty Data" to be honest...
There was a problem hiding this comment.
Do you have any suggestion? Thanks!
There was a problem hiding this comment.
Not at the moment... I didn't do too much thinking to be honest. But maybe this should be a new/separate API instead of shoehorning it into the existing API with a bool as discriminator.
There was a problem hiding this comment.
Yeah... But it's better to be compatible with old APIs because of existing apps...
| bool isPacketSubscription; | ||
| bool prefetch; | ||
| std::shared_ptr<Regex> regex = make_shared<Regex>("^<>+$"); | ||
| bool autofetch = true; |
There was a problem hiding this comment.
This wastes space in the struct, please move the bool next to the others above to minimize padding. Even better, the bools and the uint32_t should be grouped together.
ndn-svs/svspubsub.cpp
Outdated
| for (const auto& sub : m_regexSubscriptions) | ||
| { | ||
| if (sub.regex->match(mapping.first)) | ||
| if (sub.regex->match(mapping.first) and sub.autofetch) |
2 participants
A new API to publish names only (Won't change old APIs);
Subscribe with Regex and users can decide whether fetching data automatically;