Project

General

Profile

Actions

Task #2098

closed

Mark common.hpp as implementation detail

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

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

100%

Estimated time:
1.00 h

Description

Add comments to common.hpp to clarify that this file is implementation detail, and the aliases imported in this file SHOULD NOT be used outside of ndn-cxx.


Related issues 2 (0 open2 closed)

Related to ndn-cxx - Task #2099: Move definition of MAX_NDN_PACKET_SIZEClosedJunxiao Shi

Actions
Related to NLSR - Task #3983: Get rid of aliases that are imported in ndn-cxx/common.hpp.ClosedDamian Coomes03/01/2017

Actions
Actions #1

Updated by Junxiao Shi over 9 years ago

  • Related to Task #2099: Move definition of MAX_NDN_PACKET_SIZE added
Actions #2

Updated by Davide Pesavento over 9 years ago

Comments are useless, we should find a way to actually enforce this. I can think of two possibilities right now:

  • (1) Add something like the following to common.hpp
#ifndef _NDN_CXX_BUILD
#  error "This header file is private. Do not include it in your project bla bla..."
#endif

and then build ndn-cxx itself with -D_NDN_CXX_BUILD.

  • (2) Make sure common.hpp is not included from any ndn-cxx public header and stop installing it altogether.

I prefer the second way.

Actions #3

Updated by Junxiao Shi over 9 years ago

common.hpp serves three purposes:

  1. provide MAX_NDN_PACKET_SIZE constant
  2. include commonly used system headers, so that other files in ndn-cxx don't have to include them one by one
  3. import identifiers into namespace ndn to use as aliases within ndn-cxx

Purpose 1 will be relieved in Task #2099.

Purpose 2 is useful, and should be kept.
This implies that common.hpp must be installed, will be included in headers, and cannot require -D_NDN_CXX_BUILD.

Purpose 3 is previously useful because boost types are used in C++03 mode and std types are used in C++11 mode.
Those aliases are the only way for ndn-cxx to reference the correct type.
But these aliases are not needed any more because ndn-cxx can directly write std::shared_ptr etc.

However, aliases are heavily used throughout the library such that rewriting everything as std::shared_ptr is a lot of work.

Therefore, I suggest that for v0.3:

  • in ndn-cxx, just add a comment to state that aliases are implementation detail and should not be used from other projects.
  • new code in dependent projects can use alias, but cannot use ndn::shared_ptr etc explicitly; otherwise, -1 at code review.

I disagree to get rid of aliases now.

I'm indifferent at whether to forbid new code from using aliases.

Actions #4

Updated by Alex Afanasyev over 9 years ago

There is no much value in prohibiting anybody from using ndn-cxx/commmon.hpp. It was useful before (the library created a proper alias to std of boost) and it could be useful in the future. We shouldn't encourage it anymore, but I see no gains from it, only time loss in actually implementing such enforcement and then fixing numerous compilation error in dependent software.

Actions #5

Updated by Davide Pesavento over 9 years ago

The "value" is having a clear, well-defined (and documented) API. But maybe you don't care about that yet.

Actions #6

Updated by Alex Afanasyev over 9 years ago

I cannot disagree that having documented API is valuable. However, this doesn't mean we should do some enforcement to prevent others from using some internal library features, especially when we (I) have encouraged that in the past.

It should be enough mentioning in the documentation and shown in examples. I don't think there is anything else needed.

Actions #7

Updated by Junxiao Shi over 9 years ago

20141103 conference call decides just to put in a comment. No enforcement is necessary.

Future code reviews of NFD and other projects using ndn-cxx should permit shared_ptr and std::shared_ptr, but reject ndn::shared_ptr.

Actions #8

Updated by Junxiao Shi over 9 years ago

  • Status changed from New to In Progress
  • Assignee set to Junxiao Shi
  • Start date deleted (10/30/2014)
Actions #9

Updated by Junxiao Shi over 9 years ago

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

Updated by Junxiao Shi over 9 years ago

  • Status changed from Code review to Closed
Actions #11

Updated by Muktadir Chowdhury about 7 years ago

  • Related to Task #3983: Get rid of aliases that are imported in ndn-cxx/common.hpp. added
Actions

Also available in: Atom PDF