re-order includes (fixes #3132) #3133
Conversation
jameslamb
requested review from
Laurae2,
btrotta,
chivee and
huanzhang12
as code owners
| #include <sstream> | ||
| #include <utility> | ||
|
|
||
| #include <LightGBM/application.h> |
There was a problem hiding this comment.
According to the Google style, LightGBM/application.h (the same issue with the most cpp files from this PR) should be the first one. Is there a bug in cpplint which blocks us from using the correct order? Refer to #2066 (comment).
There was a problem hiding this comment.
Oh thanks, I didn't know about #2066!
Yes, if I move just LightGBM/application.h back to the top and leave everything else the same in this PR, I get this from cpplint
src/application/application.cpp:8: Found C++ system header after other system header. Should be: application.h, c system, c++ system, other. [build/include_order] [4]
src/application/application.cpp:9: Found C++ system header after other system header. Should be: application.h, c system, c++ system, other. [build/include_order] [4]
src/application/application.cpp:10: Found C++ system header after other system header. Should be: application.h, c system, c++ system, other. [build/include_order] [4]
src/application/application.cpp:11: Found C++ system header after other system header. Should be: application.h, c system, c++ system, other. [build/include_order] [4]
src/application/application.cpp:12: Found C++ system header after other system header. Should be: application.h, c system, c++ system, other. [build/include_order] [4]
src/application/application.cpp:13: Found C++ system header after other system header. Should be: application.h, c system, c++ system, other. [build/include_order] [4]
src/application/application.cpp:14: Found C++ system header after other system header. Should be: application.h, c system, c++ system, other. [build/include_order] [4]
I think it is probably because this is called application/application.cpp and cpplint is expecting to find "application.h" (header colocated with implementation). When I change <LightGBM/application.h> to "application.h" (and leave everything else in this PR the same), cpplint does not raise any warnings.
If we want to stick to the intent of the style guide, I guess we could make (for example) LightGBM/application.h the first header and just add //NOLINT. That would be closer to what the style guide says, even if cpplint fails to pick it up. What do you think?
There was a problem hiding this comment.
Yes, if I move just
LightGBM/application.hback to the top and leave everything else the same in this PR, I get this fromcpplint
Oh, it's a pity! But thanks, this is exactly what I wanted to know.
What do you think?
I think that adding //NOLINT in each file is very ugly "fix" and makes no sense. Quoting myself from #2066
IMHO, any order is OK in case it's the same across the whole project.
I suggest only document our own new style guide for includes. So that we will be able to use it as a reference for new files in the future. Just like I did it earlier:
But due to the bug only the following order doesn't produce errors:
dir2/foo2.h.
A blank line
Your project's .h files in include folder.
A blank line
C system files.
C++ system files.
A blank line
Other libraries' .h files.
Your project's .h files in src folder.
There was a problem hiding this comment.
To be clear, do you mean I should push a commit adding this new standard somewhere? If so could you suggest where?
There was a problem hiding this comment.
Ah, no! I mean, just post it here as a comment.
…icrosoft#3153) This reverts commit 656d267.
…icrosoft#3153) This reverts commit 656d267.
…icrosoft#3153) This reverts commit 656d267.
…icrosoft#3153) This reverts commit 656d267.
* Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * Initial CUDA work * redirect log to python console (#3090) * redir log to python console * fix pylint * Apply suggestions from code review * Update basic.py * Apply suggestions from code review Co-authored-by: Nikita Titov <nekit94-08@mail.ru> * Update c_api.h * Apply suggestions from code review * Apply suggestions from code review * super-minor: better wording Co-authored-by: Nikita Titov <nekit94-08@mail.ru> Co-authored-by: StrikerRUS <nekit94-12@hotmail.com> * re-order includes (fixes #3132) (#3133) * Revert "re-order includes (fixes #3132) (#3133)" (#3153) This reverts commit 656d267. * Missing change from previous rebase * Minor cleanup and removal of development scripts. * Only set gpu_use_dp on by default for CUDA. Other minor change. * Fix python lint indentation problem. * More python lint issues. * Big lint cleanup - more to come. * Another large lint cleanup - more to come. * Even more lint cleanup. * Minor cleanup so less differences in code. * Revert is_use_subset changes * Another rebase from master to fix recent conflicts. * More lint. * Simple code cleanup - add & remove blank lines, revert unneccessary format changes, remove added dead code. * Removed parameters added for CUDA and various bug fix. * Yet more lint and unneccessary changes. * Revert another change. * Removal of unneccessary code. * temporary appveyor.yml for building and testing * Remove return value in ReSize * Removal of unused variables. * Code cleanup from reviewers suggestions. * Removal of FIXME comments and unused defines. * More reviewers comments cleanup. * More reviewers comments cleanup. * More reviewers comments cleanup. * Fix config variables. * Attempt to fix check-docs failure * Update Paramster.rst for num_gpu * Removing test appveyor.yml * Add �CUDA_RESOLVE_DEVICE_SYMBOLS to libraries to fix linking issue. * Fixed handling of data elements less than 2K. * More reviewers comments cleanup. * Removal of TODO and fix printing of int64_t * Add cuda change for CI testing and remove cuda from device_type in python. * Missed one change form previous check-in * Removal AdditionConfig and fix settings. * Limit number of GPUs to one for now in CUDA. * Update Parameters.rst for previous check-in * Whitespace removal. * Cleanup unused code. * Changed uint/ushort/ulong to unsigned int/short/long to help Windows based CUDA compiler work. * Lint change from previous check-in. * Changes based on reviewers comments. * More reviewer comment changes. * Adding warning for is_sparse. Revert tmp_subset code. Only return FeatureGroupData if not is_multi_val_ * Fix so that CUDA code will compile even if you enable the SCORE_T_USE_DOUBLE define. * Reviewer comment cleanup. * Replace warning with Log message. Removal of some of the USE_CUDA. Fix typo and removal of pragma once. * Remove PRINT debug for CUDA code. * Allow to use of multiple GPUs for CUDA. * More multi-GPUs enablement for CUDA. * More code cleanup based on reviews comments. * Update docs with latest config changes. Co-authored-by: Gordon Fossum <fossum@us.ibm.com> Co-authored-by: ChipKerchner <ckerchne@linux.vnet.ibm.com> Co-authored-by: Guolin Ke <guolin.ke@outlook.com> Co-authored-by: Nikita Titov <nekit94-08@mail.ru> Co-authored-by: StrikerRUS <nekit94-12@hotmail.com> Co-authored-by: James Lamb <jaylamb20@gmail.com>
|
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
2 participants
See #3132 for details. The
linttask in CI is broken right now because of a new release tocpplinta few hours ago. As of that release,cpplintnow enforces the Google style guide recommendations for howincludes should be ordered:And the reason this is recommended: