Project

General

Profile

Actions

Task #5186

closed

Name API improvements

Added by Davide Pesavento almost 3 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Category:
Base
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:

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

Actions #1

Updated by Davide Pesavento almost 3 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 0 to 100
Actions #2

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

Actions #3

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.

Actions #4

Updated by Davide Pesavento over 2 years ago

  • Target version changed from 0.8.0 to 0.8.1
Actions #5

Updated by Davide Pesavento over 2 years ago

  • Tags set to code-cleanup
Actions #6

Updated by Davide Pesavento over 2 years ago

  • Status changed from In Progress to Code review
Actions #7

Updated by Davide Pesavento over 2 years ago

Davide Pesavento wrote:

we have another append() overload that takes a Component and [...] Component is a subclass of Block

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, and append(type, value), where value can be anything convertible to span<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.

Actions #9

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)
Actions #10

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.

no.3: https://gerrit.named-data.net/c/ndn-cxx/+/6680

Actions #11

Updated by Davide Pesavento over 2 years ago

  • Description updated (diff)
Actions #12

Updated by Davide Pesavento over 2 years ago

  • Status changed from Code review to Closed
Actions

Also available in: Atom PDF