-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[ci][c++] fixed build/header_guard
errors from cpplint
#7055
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
#ifndef SRC_NETWORK_LINKERS_H_ | ||
#define SRC_NETWORK_LINKERS_H_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line with the INCLUDE_
definitions, shouldn't this be
#ifndef SRC_NETWORK_LINKERS_H_ | |
#define SRC_NETWORK_LINKERS_H_ | |
#ifndef SRC_LIGHTGBM_NETWORK_LINKERS_H_ | |
#define SRC_LIGHTGBM_NETWORK_LINKERS_H_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cpplint expects exactly these variables...
E.g.
...
src/treelearner/gpu_tree_learner.h:5: #ifndef header guard has wrong style, please use: SRC_TREELEARNER_GPU_TREE_LEARNER_H_ [build/header_guard] [5]
src/treelearner/gpu_tree_learner.h:287: #endif line should be "#endif // SRC_TREELEARNER_GPU_TREE_LEARNER_H_" [build/header_guard] [5]
...
include/LightGBM/utils/random.h:5: #ifndef header guard has wrong style, please use: INCLUDE_LIGHTGBM_UTILS_RANDOM_H_ [build/header_guard] [5]
include/LightGBM/utils/random.h:117: #endif line should be "#endif // INCLUDE_LIGHTGBM_UTILS_RANDOM_H_" [build/header_guard] [5]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for the explanation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify this... cpplint
is just translating the directory structure into these identifiers. This file is at src/network/
, not src/LightGBM/network/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait actually, before we merge... I thought about this more and realized that not having LIGHTGBM_
or similar in the name increases the risk of conflicts with definitions from other projects.
@StrikerRUS could you try the suggestion from cpplint/cpplint#44 (comment), to see if it'd result in cpplint
being OK with everything prefixed with LIGHTGBM_
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me! I just recently spent time typing out a review comment on a PR about this (#7039 (comment)), I'm happy to have a linter catch it automatically instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, just thought about this and I'm worried that generic names like SRC_APPLICATION_PREDICTOR_HPP_
could risk conflicting with other projects.
Refer to #7002.
I believe this linting rule is quite important because it helps to catch things like