Project

General

Profile

Actions

Bug #3070

closed

Block::remove unexpected semantics

Added by Junxiao Shi over 9 years ago. Updated over 9 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Base
Target version:
Start date:
07/26/2015
Due date:
% Done:

100%

Estimated time:
1.50 h

Description

Block class has a remove method:

void
remove(uint32_t type);

This method is missing Doxygen, is missing a test case, and is unused within ndn-cxx.

Based on function name and signature, either of the following semantics are reasonable:

/** \brief remove all subelements of \p type
 *  \param type TLV-TYPE of subelement to remove
 *  \pre parse() has been invoked
 */

/** \brief remove the first subelements of \p type
 *  \param type TLV-TYPE of subelement to remove
 *  \pre parse() has been invoked
 */

However, the actual behavior is: remove all subelements whose TLV-TYPE is not type.

This semantics is surprising.

To solve this bug:

  1. decide on the correct semantics
  2. add Doxygen
  3. add test case
  4. fix implementation
  5. post a "potential breaking change" notice to ndn-lib mailing list
  6. fix regressions on dependent projects, if any

Related issues 1 (0 open1 closed)

Blocks ndn-cxx - Feature #2879: NDNLPv2: Packet and FieldsClosedEric Newberry06/10/2015

Actions
Actions #1

Updated by Junxiao Shi over 9 years ago

Actions #2

Updated by Junxiao Shi over 9 years ago

  • Assignee set to Eric Newberry

I prefer the first semantics, ie. remove all subelements of type, because std::multiset::erase(key) also removes all elements.

Actions #3

Updated by Eric Newberry over 9 years ago

  • Status changed from New to In Progress
Actions #4

Updated by Eric Newberry over 9 years ago

I agree with note-2 and I plan to implement it this way.

Actions #5

Updated by Eric Newberry over 9 years ago

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

Updated by Eric Newberry over 9 years ago

How would we do the announcement to the mailing list when the time comes? Is there any particular message that needs to be included>

Actions #7

Updated by Junxiao Shi over 9 years ago

How would we do the announcement to the mailing list when the time comes? Is there any particular message that needs to be included?

There's no particular requirement on what to include in the announcement.
One example is http://www.lists.cs.ucla.edu/pipermail/ndn-lib/2014-November/000211.html

Actions #8

Updated by Eric Newberry over 9 years ago

In the message to the ndn-lib group, should I mention that the change has already been merged?

Actions #9

Updated by Eric Newberry over 9 years ago

Please disregard note 8.

The message has been sent to ndn-lib.

Actions #10

Updated by Junxiao Shi over 9 years ago

  • Status changed from Code review to Closed
  • % Done changed from 0 to 100
Actions

Also available in: Atom PDF