NFD » ndn-cxx

What should NackReasonConcrete check for using the concept check?

• T is a subclass of NackReason.
• T::TlvType is a specialization of std::integral_constant<uint64_t, ..>.

Do I need to implement the Duplicate and GiveUp classes or just create headers for them?

Do I need to implement the Duplicate and GiveUp classes or just create headers for them?

According to design, Duplicate and GiveUp are typedefs that use NackReasonEmptyElement template.

They do not have implementation, but need unit testing.

NackReasonEmptyElement template needs implementation (in the header file), but does not need unit testing.

Do any of the components in this task need implementation in a *.cpp file or just headers?

Also, for Duplicate and GiveUp, what should I test? I assume the functions, like wireEncode(), wireDecode(), getType()?

Do any of the components in this task need implementation in a *.cpp file?

Non-virtual methods of NackReason base type go into .cpp

for Duplicate and GiveUp, what should I test?

Test all methods.

Encoded octets should be compared to hard-coded reference octets, to detect unexpected encoding changes in the future.
Look at test suite of any TLV abstraction type to see how this is done.

When I attempt to compile, I recieve this error:

error: templates may not be 'virtual'

about template<typename Encoder> virtual size_t NackReason::wireEncode(Encoder& encoder) = 0

In addition, I'm not certain how to implement NackReason::getType(), as the type of the Nack is defined in a subclass and not in NackReason. Apologies if this is simple or doesn't make sense, I'm new to templates and many of the more advanced features of C++.

error: templates may not be 'virtual'

I didn't know C++ has this limitation.
I'll see how API design can be refined to make this work.

Another part of the spec that I don't understand is NackReason::getType(). How does this function return the NackReasonType from NackReason when the type is only declared in NackReasonEmptyElement?

The same goes for NackReason::wireEncode(). There's no protected Block member to encode (if I'm understanding the ndn-cxx api correctly).

The same goes for NackReason::wireEncode(). There's no protected Block member to encode (if I'm understanding the ndn-cxx api correctly).

wireEncode() is now moved to subclasses.

When there's no Block m_wire member field, Block wireEncode() works but const Block& wireEncode() doesn't.
WireEncodable concept accepts either.

How do wireEncode and wireDecode work? What are they encoding and how are they encoding it?

How do wireEncode and wireDecode work? What are they encoding and how are they encoding it?

A NackReason concrete class (eg. Duplicate) should encode to and decode from a reason element (eg. Duplicate).

I won't answer "how". Look at any TLV abstraction type (eg. ndn::nfd::FaceStatus) for an example.

The current implementation http://gerrit.named-data.net/#/c/2123/8 is way over-complicated.

There should be just a single header/implementation file (nack.hpp/nack.cpp, with some external code definitions in some header).

We should have only one real abstraction: lp::Nack. Within lp::Nack should have access to internal "Block" which would represent a specific nack type; plus some helper methods to set/unset codes.

I disagree with note-20.

NackReason abstraction is necessary because a reason element can contain sub elements, and what sub elements are permitted to appear within a reason element differs by the type of this reason element.

What I have suggested does not preclude having sub-elements when/if we need them. What we implementing here should be general enough to not require any additional changes (new elements inside Nack block) to support any new features that we or somebody else wants.

For me, Nack block looks very much like SignatureInfo and should have a similar implementation.

I don't fully understand note-22.

Please give a complete API design and example, similar to #2841.

Here is what I would like to have for NACK. This includes a small modification of the specification of the NACK block (https://gist.github.com/cawka/f8bb5f558568f06c78ca/revisions)

enum {
  NackUnknown = 0,
  NackDuplicate = 1,
  NackGiveUp = 2
}

// https://gist.github.com/cawka/f8bb5f558568f06c78ca#network-nack
// Changes: https://gist.github.com/cawka/f8bb5f558568f06c78ca/revisions

/** \brief represents a Network NACK header
*/
class NackHeader
{
public:
  NackHeader();

  NackHeader(const Block& block);

  template<encoding::Tag TAG>
  size_t
  wireEncode(EncodingImpl<TAG>& block) const;

  const Block&
  wireEncode() const;

  void
  wireDecode(const Block& wire);

public: // getter/setters

  uint64_t
  getReason() const;

  NackHeader&
  setReason(uint64_t reason);

  std::list<Block>&
  getAllInfo();

  void
  clearAllInfo();

  NackHeader&
  addInfo(const Block&);

  void
  removeInfo(uint32_t type);

  /**
   * @brief Helper t
   */
  template<class T=Block>
  Block
  getInfo(uint64_t type);

private:
  uint32_t m_reason;
  std::list<Block> m_nackInfo;
};

In regard to note-24:

I like the use of NACK reason codes better than reason elements. They provide for easier expansion in the future, as adding additional reason codes requires less additional coding than reason elements.

However, why is uint64_t used for the getters and setters, but the reason code is stored in a uint32_t? Also, does getAllInfo() get a set of Blocks, one for each existing reason code?

Please do not hurry with coding. The design choice needs to be discussed in an NFD call.

@note 26:

I was giving feedback on the design Alex suggested. I'll wait until a design has been decided upon before I start coding.

For removeInfo/getInfo there should be uint32_t (as it is what we are using for TLV type), so I made a mistake here. For reason, I would also use 32-bit, but I don't have strong preferences.