Project

General

Profile

Actions

Task #4203

closed

code-style: amend rule 3.4

Added by Davide Pesavento over 7 years ago. Updated over 7 years ago.

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

100%

Estimated time:

Description

There is no reason to require a certain order between groups.
The only meaningful rule we can keep is to include same-project headers first.

Actions #1

Updated by Junxiao Shi over 7 years ago

I neither agree nor disagree with the removal.

I want to point out that Davide, who proposed the removal, used to suggest sorting the #include lines:
https://gerrit.named-data.net/#/c/3224/2/tools/nfdc/main.cpp@26
https://gerrit.named-data.net/#/c/2612/4/tests/daemon/face/tcp-transport.t.cpp@31

The requirement of sorting comes from rule 3.4. Once removed, I'll randomize the insert position of new #include lines.

Actions #2

Updated by Junxiao Shi over 7 years ago

  • Project changed from NFD to ndn-cxx
  • Category changed from Docs to Docs
  • Target version changed from v0.6 to v0.6
Actions #3

Updated by Davide Pesavento over 7 years ago

Junxiao Shi wrote:

I want to point out that Davide, who proposed the removal, used to suggest sorting the #include lines:
https://gerrit.named-data.net/#/c/3224/2/tools/nfdc/main.cpp@26
https://gerrit.named-data.net/#/c/2612/4/tests/daemon/face/tcp-transport.t.cpp@31

You're taking things out of context as usual... Moreover, what you're saying is simply wrong (and given your tone, I'm not sure if this was a honest mistake or you're doing it on purpose to discredit me), because: 1) I was suggesting to sort alphabetically within each include group, so a completely different kind of sorting than that of rule 3.4; and 2) I asked for that based on a common pattern that can be found in the vast majority of NFD's source files, not because of some stupid rule. And again, consistency and common sense are more important than hard rules.

Once removed, I'll randomize the insert position of new #include lines.

This is utter nonsense and doesn't warrant a response.

Actions #4

Updated by Davide Pesavento over 7 years ago

FTR, I'm fine with alphabetical sorting within each include group. What I disagree with is the sort criterion of rule 3.4, i.e. "sorted by their hierarchical position in the system with low level files included first".

Actions #5

Updated by Ashlesh Gawande over 7 years ago

In NLSR's case, if we move the lower level header first - it breaks the compilation:
https://github.com/named-data/NLSR/blob/f6bfb14deffaa555f11c256239a3230954320261/src/nlsr-runner.hpp#L30

Actions #6

Updated by Junxiao Shi over 7 years ago

Rule 3.4 covers 3 parts:

  • Grouping
  • Sorting within group
  • Ordering between groups

Please revise the proposal to only remove a certain part, if that's what you mean.

Actions #7

Updated by Davide Pesavento over 7 years ago

Let me add, again for the record, that I filed this bug just because you recently started enforcing the "sorted by their hierarchical position in the system with low level files included first" part of the rule, which is basically the opposite of what we've been doing so far.

Actions #8

Updated by Davide Pesavento over 7 years ago

Junxiao Shi wrote:

Rule 3.4 covers 3 parts:

  • Grouping

I'm fine with grouping but I don't care if it's removed.

  • Sorting within group

The current rule doesn't talk about this.

  • Ordering between groups

This is the main point I want removed, or converted into only "include same-project headers first".

Actions #9

Updated by Junxiao Shi over 7 years ago

  • Subject changed from Delete code style rule 3.4 to code-style: amend rule 3.4
  • Description updated (diff)
  • Status changed from New to In Progress
  • Assignee set to Junxiao Shi
Actions #10

Updated by Junxiao Shi over 7 years ago

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

Updated by Ashlesh Gawande over 7 years ago

In NLSR project we cannot move the local headers before ndn-cxx or boost headers without breaking compilation:
https://github.com/named-data/NLSR/blob/d5c1a378d19ac33da7220462cbd5cac3a10a30fb/src/conf-file-processor.cpp#L28

Actions #12

Updated by Junxiao Shi over 7 years ago

In NLSR project we cannot move the local headers before ndn-cxx or boost headers without breaking compilation:

If you are referring to Boost placeholders conflict (#2109), it shouldn't break anything as long as every local header that includes a Boost header also includes a ndn-cxx header before it. The easiest way to achieve this is to have a common.hpp which includes commonly used ndn-cxx headers before Boost headers.

Actions #13

Updated by Ashlesh Gawande over 7 years ago

Okay that's what the conflict is. Thanks.

Actions #14

Updated by Junxiao Shi over 7 years ago

  • Status changed from Code review to Closed
Actions

Also available in: Atom PDF