NFD » ndn-cxx

Junxiao Shi wrote:

ndn-tools, repo-ng, and traffic-generator are unaffected https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/248309680.

ndn-tools is affected, see https://travis-ci.org/named-data/ndn-tools/builds/248956624

Looks like ndn-cxx-breaks doesn't build with tests enabled.

ndn-tools is affected

https://gerrit.named-data.net/3984

Looks like ndn-cxx-breaks doesn't build with tests enabled.

ndn-tools does not have --with-tests on 11JUN2015, so ndn-cxx-breaks does not have it.
It's added now.

https://gerrit.named-data.net/3990 moves util/signal-*.*.
My question is, should I change namespace from ndn::util::signal to ndn::signal? The util part seems unnecessary given everything is already in a signal namespace. ndn::time, for example, does not have util part.

I'm ok either way, with a slight preference of keeping the "util" namespace to reduce semi-pointless refactoring in the dependent code.

I have a slight preference for removing util::, but we can keep type aliases in ndn::util to avoid breaking other projects (or vice versa, keep them in ndn::util and add aliases in ndn namespace).

I'm usually aggressive in refactoring, so I've adopted ndn::signal::* and ndn::Signal in https://gerrit.named-data.net/3990 patchset2.
The old identifiers are deprecated.

An unfortunate side-effect of deprecating ndn::util::Signal is that, if Signal is used in code that itself is under ndn::util, a false deprecated warning would appear. This affects ndn::util::DummyClientFace and ndn::util::NotificationSubscriber.
I've changed the usages in headers to ndn::Signal to avoid polluting the output in dependant projects, but left them as is for .cpp and test suites.

Junxiao Shi wrote:

The old identifiers are deprecated.

I don't think there's any need to deprecate them now. The maintenance cost is zero, and as Alex said it would just trigger an avalanche of pointless changes in dependent projects. You may use doxygen to document the old aliases as deprecated though.

I disagree with having 3 names for the same Signal type. Thus, I'll revert to ndn::util::Signal.

You may use doxygen to document the old aliases as deprecated though.

This is useless. Based on past experience, dependant projects will NEVER notice until there's a warning/error, regardless of how many "deprecation notice" emails I send to ndn-lib.

move in-memory-storage-* to its own subdir

This one is going to get ugly. The class name is InMemoryStorageLru. Changing it to in_memory_storage::Lru is bad because the class name itself Lru does not indicate it is-a InMemoryStorage but feels like a tag.
Without changing the class name, rule 3.2 forces the header file to be named in-memory-storage-lru.hpp, and the include line becomes #include <ndn-cxx/util/in-memory-storage/in-memory-storage-lru.hpp> which repeats "in-memory-storage" twice.

Junxiao Shi wrote:

move in-memory-storage-* to its own subdir

This one is going to get ugly. The class name is InMemoryStorageLru. Changing it to in_memory_storage::Lru is bad because the class name itself Lru does not indicate it is-a InMemoryStorage but feels like a tag.

Who said anything about changing the class name?

Without changing the class name, rule 3.2 forces the header file to be named in-memory-storage-lru.hpp, and the include line becomes #include <ndn-cxx/util/in-memory-storage/in-memory-storage-lru.hpp> which repeats "in-memory-storage" twice.

Self-inflicted problems. Besides, "in-memory-storage" is kind of long for a directory name, I suggest something shorter like "ims" or "memstore" or similar...

why not moving it up one level to src/ims ?

I can, but do I also change the namespace? Should types be named like ndn::ims::InMemoryStorageLru, or even ndn::InMemoryStorageLru?

An ims namespace doesn't seem to be necessary, but I'm fine either way.

Junxiao Shi wrote:

dummy-client-face can move to a new src/dummyfw/ subdir together with the dummy forwarder being developed in #3913

This can be performed after #3913 is done.

sqlite3-statement feel like they should go into a detail/ subdir somewhere, unless they're already used outside the library

Although sqlite3-statement is unused outside ndn-cxx, it shall be useful to simplify repo-ng and ndns where SQLite3 database is used.

Fair enough.

What about:

move crypto and digest to src/security/

crypto.hpp is deprecated and will be removed soon, so there's no point in moving it around. digest.hpp should be moved.

Btw, the pib tool in ndn-tools uses in-memory-storage, so it's now broken (although it's already not built by default IIRC).

Junxiao Shi wrote:

• SHA256 algorithm itself is not specific to NDN security (signing and verifications). It is a general utility. Its use case out of security includes data disambiguation via ImplicitSha256DigestComponent.

Several things in src/security/ are not specific to NDN security... e.g. the whole transform framework... So this is not really a valid point.

• Placing a SHA256 algorithm helper together with signature types that involve SHA256 such as signature-sha256-with-rsa.hpp would be confusing.

Why would it be confusing? It's no more confusing than the difference between the two algorithms, or the difference between RSA and ECDSA, or virtually anything security-related. If one doesn't know the difference between a signature and a digest, they shouldn't be touching security-related things altogether, everything would look confusing to them. So this point is also invalid.

ndn-tools (excluding disabled PIB) and traffic-generator are unaffected https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/256747285.

You can't ignore the pib tool just because it's not built by default.

Davide Pesavento wrote:

Junxiao Shi wrote:

• Placing a SHA256 algorithm helper together with signature types that involve SHA256 such as signature-sha256-with-rsa.hpp would be confusing.

Why would it be confusing? It's no more confusing than the difference between the two algorithms, or the difference between RSA and ECDSA, or virtually anything security-related. If one doesn't know the difference between a signature and a digest, they shouldn't be touching security-related things altogether, everything would look confusing to them. So this point is also invalid.

Something that can cause confusion is the difference between security/digest-sha256.hpp and util/sha256.hpp, and this confusion can arise regardless of the subdir in which the headers reside.

20170724 NFD call decides that Sha256 can stay in util/.

Just for the record. I have no objections of renaming to sha256.hpp and keeping it in util.

https://gerrit.named-data.net/c/NLSR/+/7125/2 namespace change in NLSR.

https://gerrit.named-data.net/c/PSync/+/7131 namespace change in PSync

https://gerrit.named-data.net/c/ndn-tools/+/7134 namespace change in ndn-tools