Skip to content
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

[ci] fixed cpplint errors about namespace using-directives #2927

Merged
merged 1 commit into from
Mar 21, 2020

Conversation

jameslamb
Copy link
Collaborator

In this PR, I tried to address the two instances of this cpplint error (from #1990):

Do not use namespace using-directives. Use using-declarations instead. [build/namespaces] [5]

I did some research tonight, and found some good resources that explain this. According to this blog post:

The vast majority of C++ users think that the using-directive is injecting names into the scope where it’s declared.... [for example] the scope of [a] function. In reality, the names are injected into the nearest common ancestor of the target namespace ...and the usage namespace ... [which could easily be] the global namespace!

So a directive like using namespace LightGBM is risky because it means all the names from the namespace LightGBM will be defined in some shared parent namespace of LightGBM and the target scope. That increases the likelihood of naming conflicts.

A using-declaration, like using LightGBM::Booster, is safer, because it targets a single name. From cppreference:

Using-declaration introduces a member of another namespace into current namespace or block scope

These changes will take us down to 40 cpplint errors (42 in the log below because #2920 is awaiting review):

cpplint log (click me) Done processing src/boosting/gbdt_model_text.cpp Done processing src/boosting/gbdt.h Done processing src/network/network.cpp Done processing src/treelearner/serial_tree_learner.h Done processing src/objective/xentropy_objective.hpp Done processing src/metric/multiclass_metric.hpp Done processing src/metric/dcg_calculator.cpp Done processing src/objective/rank_objective.hpp Done processing src/io/metadata.cpp Done processing include/LightGBM/dataset_loader.h Done processing src/treelearner/data_partition.hpp Done processing src/boosting/boosting.cpp Done processing src/io/multi_val_sparse_bin.hpp Done processing include/LightGBM/export.h Done processing include/LightGBM/utils/pipeline_reader.h Done processing src/metric/binary_metric.hpp Done processing src/io/dense_bin.hpp Done processing include/LightGBM/prediction_early_stop.h Done processing src/treelearner/data_parallel_tree_learner.cpp Done processing include/LightGBM/metric.h Done processing src/c_api.cpp Done processing src/network/linkers_mpi.cpp Done processing src/metric/xentropy_metric.hpp Done processing src/metric/rank_metric.hpp include/LightGBM/utils/common.h:316: Use int16/int64/etc, rather than the C type long [runtime/int] [4] include/LightGBM/utils/common.h:387: Never use sprintf. Use snprintf instead. [runtime/printf] [5] include/LightGBM/utils/common.h:432: Never use sprintf. Use snprintf instead. [runtime/printf] [5] Done processing include/LightGBM/utils/common.h Done processing include/LightGBM/utils/array_args.h Done processing src/treelearner/voting_parallel_tree_learner.cpp Done processing src/io/bin.cpp Done processing include/LightGBM/dataset.h Done processing src/boosting/score_updater.hpp Done processing src/io/config.cpp Done processing src/io/sparse_bin.hpp Done processing include/LightGBM/utils/threading.h Done processing include/LightGBM/application.h Done processing src/io/config_auto.cpp Done processing src/io/dataset_loader.cpp Done processing include/LightGBM/tree.h Done processing src/metric/map_metric.hpp Done processing src/treelearner/serial_tree_learner.cpp Done processing src/boosting/dart.hpp Done processing include/LightGBM/tree_learner.h Done processing src/io/parser.cpp Done processing src/network/linkers_socket.cpp Done processing src/treelearner/cost_effective_gradient_boosting.hpp src/io/dataset.cpp:437: Is this a non-const reference? If so, make const or use a pointer: std::vector>>& iters [runtime/references] [2] Done processing src/io/dataset.cpp Done processing src/lightgbm_R.cpp Done processing src/boosting/gbdt.cpp Done processing src/main.cpp src/network/socket_wrapper.hpp:221: Are you taking an address of a cast? This is dangerous: could be a temp var. Take the address before doing the cast, rather than after [runtime/casting] [4] Done processing src/network/socket_wrapper.hpp Done processing src/metric/regression_metric.hpp Done processing include/LightGBM/network.h Done processing src/treelearner/gpu_tree_learner.h Done processing include/LightGBM/utils/log.h Done processing src/boosting/goss.hpp Done processing src/objective/regression_objective.hpp Done processing src/boosting/prediction_early_stop.cpp include/LightGBM/utils/json11.h:80: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:81: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:82: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:83: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:84: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:85: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:86: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:87: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:88: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:89: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:90: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:94: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:101: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:107: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:111: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] include/LightGBM/utils/json11.h:144: Is this a non-const reference? If so, make const or use a pointer: std::string &out [runtime/references] [2] include/LightGBM/utils/json11.h:153: Is this a non-const reference? If so, make const or use a pointer: std::string & err [runtime/references] [2] include/LightGBM/utils/json11.h:156: Is this a non-const reference? If so, make const or use a pointer: std::string & err [runtime/references] [2] include/LightGBM/utils/json11.h:168: Is this a non-const reference? If so, make const or use a pointer: std::string::size_type & parser_stop_pos [runtime/references] [2] include/LightGBM/utils/json11.h:169: Is this a non-const reference? If so, make const or use a pointer: std::string & err [runtime/references] [2] include/LightGBM/utils/json11.h:174: Is this a non-const reference? If so, make const or use a pointer: std::string & err [runtime/references] [2] include/LightGBM/utils/json11.h:193: Is this a non-const reference? If so, make const or use a pointer: std::string & err [runtime/references] [2] include/LightGBM/utils/json11.h:208: Is this a non-const reference? If so, make const or use a pointer: std::string &out [runtime/references] [2] Done processing include/LightGBM/utils/json11.h Done processing src/io/parser.hpp Done processing include/LightGBM/lightgbm_R.h Done processing src/treelearner/gpu_tree_learner.cpp Done processing src/boosting/gbdt_prediction.cpp Done processing src/metric/metric.cpp Done processing src/io/multi_val_dense_bin.hpp Done processing src/application/application.cpp Done processing include/LightGBM/config.h Done processing include/LightGBM/R_object_helper.h Done processing include/LightGBM/feature_group.h Done processing include/LightGBM/meta.h Done processing src/objective/multiclass_objective.hpp Done processing include/LightGBM/utils/random.h Done processing include/LightGBM/utils/openmp_wrapper.h Done processing include/LightGBM/utils/locale_context.h Done processing src/network/linkers.h Done processing src/io/tree.cpp Done processing src/boosting/rf.hpp Done processing src/treelearner/monotone_constraints.hpp Done processing src/treelearner/feature_histogram.hpp Done processing src/treelearner/parallel_tree_learner.h Done processing src/treelearner/feature_parallel_tree_learner.cpp Done processing src/network/linker_topo.cpp Done processing include/LightGBM/bin.h Done processing src/objective/objective_function.cpp Done processing src/io/file_io.cpp Done processing src/treelearner/col_sampler.hpp Done processing src/treelearner/leaf_splits.hpp Done processing src/treelearner/split_info.hpp include/LightGBM/c_api.h:1077: Almost always, snprintf is better than strcpy [runtime/printf] [4] Done processing include/LightGBM/c_api.h Done processing include/LightGBM/objective_function.h Done processing src/objective/binary_objective.hpp Done processing src/application/predictor.hpp Done processing include/LightGBM/utils/text_reader.h Done processing src/treelearner/tree_learner.cpp Done processing include/LightGBM/utils/file_io.h src/io/json11.cpp:55: Is this a non-const reference? If so, make const or use a pointer: string &out [runtime/references] [2] src/io/json11.cpp:59: Is this a non-const reference? If so, make const or use a pointer: string &out [runtime/references] [2] src/io/json11.cpp:69: Is this a non-const reference? If so, make const or use a pointer: string &out [runtime/references] [2] src/io/json11.cpp:75: Is this a non-const reference? If so, make const or use a pointer: string &out [runtime/references] [2] src/io/json11.cpp:79: Is this a non-const reference? If so, make const or use a pointer: string &out [runtime/references] [2] src/io/json11.cpp:116: Is this a non-const reference? If so, make const or use a pointer: string &out [runtime/references] [2] src/io/json11.cpp:128: Is this a non-const reference? If so, make const or use a pointer: string &out [runtime/references] [2] src/io/json11.cpp:341: Use int16/int64/etc, rather than the C type long [runtime/int] [4] src/io/json11.cpp:353: const string& members are dangerous. It is much better to use alternatives, such as pointers or simple constants. [runtime/member_string_references] [2] src/io/json11.cpp:454: Use int16/int64/etc, rather than the C type long [runtime/int] [4] src/io/json11.cpp:454: Is this a non-const reference? If so, make const or use a pointer: string & out [runtime/references] [2] src/io/json11.cpp:481: Use int16/int64/etc, rather than the C type long [runtime/int] [4] src/io/json11.cpp:525: Use int16/int64/etc, rather than the C type long [runtime/int] [4] Done processing src/io/json11.cpp Done processing include/LightGBM/boosting.h Total errors found: 42

@StrikerRUS StrikerRUS merged commit 3e540ea into microsoft:master Mar 21, 2020
@jameslamb jameslamb deleted the misc/using-declarations branch March 25, 2020 19:55
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants