fixed cpplint errors about string& members

[jameslamb]

In this PR, I'd like to propose a fix for this cpplint error (from #1990)

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]

Currently, json11 takes in the string to be parsed by reference:

const string& str;

From what I can tell googling around, I think that cpplint thinks this is dangerous because even though you're declaring it as const, it can be a reference to any part of memory (not just a read-only part like a const string) so there is still an opportunity to change the value at that reference.

These changes would take us down to 39 cpplint errors (43 in the log below because #2920 and #2927 are awaiting review).

cpplint log (click me)

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 src/io/sparse_bin.hpp
Done processing src/metric/xentropy_metric.hpp
Done processing src/treelearner/serial_tree_learner.cpp
Done processing src/application/application.cpp
Done processing src/io/metadata.cpp
Done processing src/metric/regression_metric.hpp
Done processing src/metric/binary_metric.hpp
Done processing src/metric/metric.cpp
Done processing src/application/predictor.hpp
Done processing include/LightGBM/prediction_early_stop.h
Done processing src/boosting/dart.hpp
Done processing src/objective/binary_objective.hpp
Done processing include/LightGBM/utils/file_io.h
Done processing src/metric/rank_metric.hpp
Done processing include/LightGBM/meta.h
Done processing src/network/linker_topo.cpp
Done processing src/metric/multiclass_metric.hpp
Done processing src/metric/map_metric.hpp
Done processing include/LightGBM/utils/openmp_wrapper.h
src/c_api.cpp:556:  Do not use namespace using-directives.  Use using-declarations instead.  [build/namespaces] [5]
Done processing src/c_api.cpp
Done processing src/network/linkers_mpi.cpp
Done processing src/boosting/boosting.cpp
Done processing include/LightGBM/export.h
Done processing src/io/dense_bin.hpp
Done processing src/boosting/gbdt_model_text.cpp
Done processing include/LightGBM/application.h
Done processing include/LightGBM/lightgbm_R.h
Done processing src/boosting/score_updater.hpp
Done processing src/io/config_auto.cpp
Done processing src/treelearner/data_parallel_tree_learner.cpp
Done processing src/treelearner/feature_parallel_tree_learner.cpp
Done processing src/boosting/prediction_early_stop.cpp
Done processing src/objective/objective_function.cpp
Done processing src/treelearner/split_info.hpp
Done processing include/LightGBM/utils/threading.h
Done processing include/LightGBM/utils/log.h
Done processing src/io/parser.hpp
Done processing src/io/config.cpp
Done processing src/io/multi_val_dense_bin.hpp
Done processing src/io/file_io.cpp
Done processing src/boosting/rf.hpp
Done processing include/LightGBM/utils/random.h
Done processing src/objective/regression_objective.hpp
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: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 src/treelearner/cost_effective_gradient_boosting.hpp
Done processing src/treelearner/feature_histogram.hpp
Done processing src/network/linkers_socket.cpp
Done processing src/metric/dcg_calculator.cpp
Done processing src/treelearner/serial_tree_learner.h
Done processing include/LightGBM/utils/locale_context.h
Done processing include/LightGBM/network.h
Done processing src/network/linkers.h
Done processing include/LightGBM/metric.h
Done processing src/boosting/gbdt_prediction.cpp
Done processing src/io/parser.cpp
Done processing include/LightGBM/utils/text_reader.h
Done processing include/LightGBM/boosting.h
Done processing src/treelearner/monotone_constraints.hpp
src/lightgbm_R.cpp:35:  Do not use namespace using-directives.  Use using-declarations instead.  [build/namespaces] [5]
Done processing src/lightgbm_R.cpp
Done processing src/treelearner/data_partition.hpp
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 include/LightGBM/dataset_loader.h
Done processing include/LightGBM/tree_learner.h
Done processing src/treelearner/parallel_tree_learner.h
Done processing src/treelearner/gpu_tree_learner.cpp
Done processing include/LightGBM/utils/pipeline_reader.h
Done processing include/LightGBM/bin.h
Done processing src/objective/rank_objective.hpp
Done processing src/treelearner/tree_learner.cpp
Done processing src/network/network.cpp
Done processing src/treelearner/voting_parallel_tree_learner.cpp
Done processing include/LightGBM/tree.h
Done processing include/LightGBM/config.h
Done processing src/io/bin.cpp
Done processing src/treelearner/gpu_tree_learner.h
Done processing src/objective/xentropy_objective.hpp
Done processing src/treelearner/leaf_splits.hpp
Done processing src/io/dataset_loader.cpp
Done processing src/boosting/gbdt.cpp
src/io/dataset.cpp:437:  Is this a non-const reference? If so, make const or use a pointer: std::vector<std::vector<std::unique_ptr<BinIterator>>>& iters  [runtime/references] [2]
Done processing src/io/dataset.cpp
Done processing include/LightGBM/dataset.h
Done processing src/boosting/gbdt.h
Done processing src/io/tree.cpp
Done processing src/io/multi_val_sparse_bin.hpp
Done processing include/LightGBM/objective_function.h
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/feature_group.h
Done processing include/LightGBM/utils/array_args.h
Done processing src/boosting/goss.hpp
Done processing src/objective/multiclass_objective.hpp
Done processing src/main.cpp
Done processing src/treelearner/col_sampler.hpp
Done processing include/LightGBM/R_object_helper.h
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
Total errors found: 43

[guolinke]

@jameslamb maybe const char* str ?

[jameslamb]

@jameslamb maybe const char* str ?

@guolinke sure I can switch to a pointer

[jameslamb]

@jameslamb maybe const char* str ?

@guolinke sure I can switch to a pointer

@guolinke I've been trying this and honestly I think changing to const char* str or const string* str is beyond my ability. If you want to do that, I think I should close this PR and leave it to someone else to pick up.

[guolinke]

I can directly try on this PR @jameslamb

[guolinke]

@jameslamb my fix is in https://github.com/microsoft/LightGBM/tree/jameslamb/cpplint/explicit-constructors, could you sync to your repo?

[jameslamb]

@guolinke I just merged your changes into my branch (and then rebased to master) and pushed here.

Unfortunately I still see the warning:

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]

[jameslamb]

close re-open to trigger CI. This PR got caught in the Travis outage

[guolinke]

@jameslamb oh, I will refix it.

updated: done.

[StrikerRUS]

Seems this PR increases number of errors to 41.

[guolinke]

@jameslamb could you update the branch ?

[jameslamb]

@jameslamb could you update the branch ?

I just merged this branch with current master.

[guolinke]

@jameslamb oh, sorry, i mean this branch: https://github.com/microsoft/LightGBM/tree/jameslamb/cpplint/explicit-constructors

[jameslamb]

@jameslamb oh, sorry, i mean this branch: https://github.com/microsoft/LightGBM/tree/jameslamb/cpplint/explicit-constructors

ah sorry! Just merged your changes into this branch: a9a2556

I checked cpplint locally after the merge. I can confirm that the error about const string& members is gone and no new errors have been introduced. Thanks @guolinke !

[jameslamb]

@guolinke the only thing I changed in your code was adding these parentheses. My editor was complaining because it interpreted that first < as the beginning of a cast instead of a logical operator 😆

[StrikerRUS]

Thank you guys!
-1 cpplint error 🙂

[github-actions]

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.