Task #3940
closed
Restructure src/util/
Added by Davide Pesavento almost 8 years ago. Updated over 1 year ago.
100%
Description
The src/util/ subdir contains a bunch of unrelated classes and functions. I suggest re-organizing it as follows:
- move dns, ethernet, network-monitor, and detail/network-monitor-* to a new
src/net/directory - face-uri can stay, or it can move to either
src/net/orsrc/, given that it's a pretty central and fundamental concept - move crypto and digest to
src/security/- hopefully we can also reduce duplication between them, see #3886
- dummy-client-face can stay, or it can move to a new
src/dummyfw/subdir together with the dummy forwarder being developed in #3913- feel free to bikeshed the exact name of the subdir
- move in-memory-storage-* to its own subdir
- move signal-* to its own subdir
- keep the convenience header
signal.hppinsrc/util/
- keep the convenience header
- monotonic_deadline_timer and sqlite3-statement feel like they should go into a
detail/subdir somewhere, unless they're already used outside the library
This leaves us with the following in src/util/: backports & backports-optional, concepts, config-file, (maybe dummy-client-face), (maybe face-uri), indented-stream, io, logger & logging, notification-stream & notification-subscriber, random, scheduler & scheduler-scoped-event-id, segment-fetcher, string-helper, time & time-{custom,unit-test}-clock.
Updated by Junxiao Shi over 7 years ago
- Assignee set to Junxiao Shi
- Target version set to v0.6
- % Done changed from 0 to 30
- Estimated time set to 5.00 h
https://gerrit.named-data.net/3979 moves network-related classes to src/net:
ndn::dnsndn::util::ethernet, renamed tondn::ethernetnamespaceNetworkMonitorand related classes, now inndn::netnamespaceFaceUri, now inndnnamespace
No compatibility headers are provided, because v0.6 is expected to be breaking.
https://gerrit.named-data.net/3981 updates NFD codebase following the above change.
Prior to that, https://gerrit.named-data.net/3980 fixes code style problems in nfd-autoreg.cpp.
ndn-tools, repo-ng, and traffic-generator are unaffected https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/248309680.
NLSR has pinned to older ndn-cxx.
ChronoSync and ChronoChat are already broken prior to this change.
Updated by Davide Pesavento over 7 years ago
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.
Updated by Junxiao Shi over 7 years ago
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.
Updated by Junxiao Shi over 7 years ago
- % Done changed from 30 to 40
https://gerrit.named-data.net/3986 moves CFReleaser to util/cf-releaser-osx.hpp and into ndn::util namespace. It was in security/tpm but is being used in NetworkMonitor::Impl on macOS platform.
Updated by Junxiao Shi over 7 years ago
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.
Updated by Alex Afanasyev over 7 years ago
I'm ok either way, with a slight preference of keeping the "util" namespace to reduce semi-pointless refactoring in the dependent code.
Updated by Davide Pesavento over 7 years ago
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).
Updated by Junxiao Shi over 7 years ago
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.
Updated by Davide Pesavento over 7 years ago
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.
Updated by Junxiao Shi over 7 years ago
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.
Updated by Junxiao Shi over 7 years ago
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.
Updated by Davide Pesavento over 7 years ago
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 toin_memory_storage::Lruis bad because the class name itselfLrudoes not indicate it is-aInMemoryStoragebut 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...
Updated by Junxiao Shi over 7 years ago
- % Done changed from 40 to 50
https://gerrit.named-data.net/4037 MonotonicDeadlineTimer
Note that MonotonicDeadlineTimer is now a subclass of boost::asio::basic_deadline_timer<time::steady_clock> instead of a typedef, because scheduler.hpp needs to forward-declare it.
Updated by Junxiao Shi over 7 years ago
- % Done changed from 50 to 70
https://gerrit.named-data.net/4046 InMemoryStorage
Updated by Junxiao Shi over 7 years ago
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?
Updated by Davide Pesavento over 7 years ago
An ims namespace doesn't seem to be necessary, but I'm fine either way.
Updated by Junxiao Shi over 7 years ago
- Status changed from In Progress to Resolved
I do not plan to do the following:
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.
This issue will close after 24 hours if there's no objection.
Updated by Davide Pesavento over 7 years ago
Junxiao Shi wrote:
dummy-client-face can move to a new
src/dummyfw/subdir together with the dummy forwarder being developed in #3913This 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 libraryAlthough
sqlite3-statementis 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.
Updated by Davide Pesavento over 7 years ago
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).
Updated by Junxiao Shi over 7 years ago
- % Done changed from 70 to 100
crypto.hppis deprecated and will be removed soon, so there's no point in moving it around.digest.hppshould be moved.
I disagree with placing Sha256 class into security/ directory.
- 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.
- Placing a SHA256 algorithm helper together with signature types that involve SHA256 such as
signature-sha256-with-rsa.hppwould be confusing.
https://gerrit.named-data.net/4061 renames util/digest.hpp to util/sha256.hpp to match the enclosed class name, as required by rule 2.1. Deprecated util/crypto.hpp is removed in the same commit.
No compatibility headers are provided, because v0.6 is expected to be breaking.
https://gerrit.named-data.net/4062 updates NFD codebase following the above change.
ndn-tools (excluding disabled PIB) and traffic-generator are unaffected https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/256747285.
NLSR has pinned to older ndn-cxx.
repo-ng, ndns, ChronoSync, and ChronoChat are already broken prior to this change.
Updated by Davide Pesavento over 7 years ago
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.hppwould 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.
Updated by Davide Pesavento over 7 years ago
Davide Pesavento wrote:
Junxiao Shi wrote:
- Placing a SHA256 algorithm helper together with signature types that involve SHA256 such as
signature-sha256-with-rsa.hppwould 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.
Updated by Junxiao Shi over 7 years ago
20170724 NFD call decides that Sha256 can stay in util/.
Updated by Alex Afanasyev over 7 years ago
Just for the record. I have no objections of renaming to sha256.hpp and keeping it in util.
Updated by Junxiao Shi over 1 year ago
https://gerrit.named-data.net/c/NLSR/+/7125/2 namespace change in NLSR.
Updated by Junxiao Shi over 1 year ago
https://gerrit.named-data.net/c/PSync/+/7131 namespace change in PSync
Updated by Junxiao Shi over 1 year ago
https://gerrit.named-data.net/c/ndn-tools/+/7134 namespace change in ndn-tools