Task #4203
closedcode-style: amend rule 3.4
100%
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.
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.
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
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.
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".
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
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.
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.
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".
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
Updated by Junxiao Shi over 7 years ago
- Status changed from In Progress to Code review
- % Done changed from 0 to 100
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
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.
Updated by Ashlesh Gawande over 7 years ago
Okay that's what the conflict is. Thanks.
Updated by Junxiao Shi over 7 years ago
- Status changed from Code review to Closed