Task #5186
closedName API improvements
100%
Description
Improvement no. 1¶
The overload of Name::append()
that takes a Block
has confusing semantics. It has a different behavior if the passed Block
is a GenericNameComponent, but does not apply the same special treatment to other typed name components. And this gets even more confusing when we consider that we have another append()
overload that takes a Component
and that Component
is a subclass of Block
. This means that appending a non-generic name component may have very different results depending on whether it's passed as a Block
or as a Component
. I propose to deprecate and eventually remove the Name::append(Block)
overload.
Improvement no. 2¶
See #note-7
Improvement no. 3¶
See #note-10
Updated by Davide Pesavento almost 3 years ago
- Status changed from In Progress to Code review
- % Done changed from 0 to 100
no.1 (deprecation): https://gerrit.named-data.net/c/ndn-cxx/+/6560
Updated by Junxiao Shi almost 3 years ago
- Status changed from Code review to Feedback
NLSR and repo-ng have several warnings due to this deprecation.
Please upload patch.
https://github.com/yoursunny/ndn-cxx-breaks/actions/runs/1646373596
Updated by Davide Pesavento almost 3 years ago
- Status changed from Feedback to In Progress
I'm well aware, flagging all uses of the function was exactly the goal of this deprecation. But I won't fix the call sites directly (unless they rely on the special case). I have a different plan in mind that involves more span
-based APIs.
Updated by Davide Pesavento almost 3 years ago
- Target version changed from 0.8.0 to 0.8.1
Updated by Davide Pesavento over 2 years ago
- Status changed from In Progress to Code review
no.1 (removal): https://gerrit.named-data.net/c/ndn-cxx/+/6678
Updated by Davide Pesavento over 2 years ago
Davide Pesavento wrote:
we have another
append()
overload that takes aComponent
and [...]Component
is a subclass ofBlock
It's actually worse than I thought because Component
is implicitly constructible from Block
. This means that, after the removal of Name::append(Block)
, the following code now triggers the implicit conversion from Block
to Component
and invokes Name::append(Component)
.
Block b = ...;
Name n;
n.append(b);
This isn't terrible (certainly better than before, at least now the semantics don't depend on the TLV type of b
), but I think it's still surprising/unclear to the reader that the append
call ends up appending the block as-is (i.e., with whatever TLV type), rather than as a GenericNameComponent (with b
becoming the TLV value of that component).
Strictly speaking, this behavior isn't wrong per se, but IMHO it's at the very least:
- inconsistent: other
append()
overloads either take the TLV-TYPE as an explicit argument, or they wrap the argument in a GenericNameComponent. The "thing" passed as argument is never appended as-is to the name. - redundant: we already have
append(Component)
to append as-is, andappend(type, value)
, wherevalue
can be anything convertible tospan<uint8_t>
, to wrap a "thing" in a NameComponent and append it to the name. - unclear: when reading code using
append
, it won't be immediately obvious what the code is doing, because overloads that look similar have in fact very different behaviors.
These reasons, coupled with the fact that no other "TLV class" has an implicit constructor from Block
, convinced me that we should make the Component(const Block&)
constructor explicit.
Updated by Davide Pesavento over 2 years ago
no.2: https://gerrit.named-data.net/c/ndn-cxx/+/6679
Only ndns is broken.
Updated by Davide Pesavento over 2 years ago
- Tags changed from code-cleanup to code-cleanup, API
- Subject changed from Deprecate and remove Name::append(Block) to Name API improvements
- Description updated (diff)
Updated by Davide Pesavento over 2 years ago
And after the two previous improvements/cleanups, we can finally introduce an overload of Name::append()
that takes a span<>
and appends a GenericNameComponent with the given value, without the risk of ambiguous overloads and unclear/surprising semantics.
Updated by Davide Pesavento over 2 years ago
- Status changed from Code review to Closed