fix MSVC warning about control paths (fixes #3067)

[jameslamb]

See #3067 for details. When building for the R package, Log::Fatal calls Rf_error:

https://github.com/microsoft/LightGBM/blob/master/include/LightGBM/utils/log.h#L123

That function will result in a fatal error, but not on the C++ side...it tells the calling R process to throw n R error. I think that is why MSVC thinks it's possible for control to reach the end of PredictionEarlyStopInstance.

In this PR, I'd like to propose refactoring PredictionEarlyStopInstance to make it clearer to compilers that this function's control flow does always end.

We don't have CI for MSVC + R builds yet (it's in progress in #2965 ), but I tested this on my laptop tonight (Windows 10, Visual Studio 16 2019, R 3.6.1, Rtools 3.5). You can see in this log from running Rscript build_r.R that the warning is gone:

[StrikerRUS]

@jameslamb I think this warning can be fixed with less diff and without requiring code modification in multiple place for further updates (with the current fix one should update first if (if (type != "none" && type != "multiclass" && type != "binary")) and only then make actual modification with new else if (type == "something_new")). Please refer to for the example: #2866.

LightGBM/include/LightGBM/prediction_early_stop.h

Lines 15 to 23 in 0a4a7a8

	struct PredictionEarlyStopInstance {
	/// Callback function type for early stopping.
	/// Takes current prediction and number of elements in prediction
	/// @returns true if prediction should stop according to criterion
	using FunctionType = std::function<bool(const double*, int)>;
	FunctionType callback_function; // callback function itself
	int round_period; // call callback_function every `runPeriod` iterations
	};

[jameslamb]

@jameslamb I think this warning can be fixed with less diff and without requiring code modification in multiple place for further updates (with the current fix one should update first if (if (type != "none" && type != "multiclass" && type != "binary")) and only then make actual modification with new else if (type == "something_new")). Please refer to for the example: #2866.

LightGBM/include/LightGBM/prediction_early_stop.h

Lines 15 to 23 in 0a4a7a8

	struct PredictionEarlyStopInstance {
	/// Callback function type for early stopping.
	/// Takes current prediction and number of elements in prediction
	/// @returns true if prediction should stop according to criterion
	using FunctionType = std::function<bool(const double*, int)>;
	FunctionType callback_function; // callback function itself
	int round_period; // call callback_function every `runPeriod` iterations
	};

😱 this is great! I'll try it.

[jameslamb]

Unfortunately the nullptr trick did not work. I used my fork of #2965 to test this on Visual Studio quickly

• build
• commit

I got this new warning:

'return': cannot convert from 'nullptr' to 'LightGBM::PredictionEarlyStopInstance'

[StrikerRUS]

Unfortunately the nullptr trick did not work.

Ah, it was expected. Return type should match function signature. I imbedded code of PredictionEarlyStopInstance definition in my previous comment which should be returned from the function you are refactoring in this PR. I guess empty PredictionEarlyStopInstance instead of nullptr should do the trick.

[jameslamb]

Unfortunately the nullptr trick did not work.

Ah, it was expected. Return type should match function signature. I imbedded code of PredictionEarlyStopInstance definition in my previous comment which should be returned from the function you are refactoring in this PR. I guess empty PredictionEarlyStopInstance instead of nullptr should do the trick.

ohhhhhhhh I misunderstood, ok I like that

[jameslamb]

Ok just pushed a change to be sure a PredictionEarlyStopInstance is returned (even though that line is never actually reached at runtime).

Seems from my fork that this works and suppresses the warning: https://dev.azure.com/JayLamb20/jameslamb-lightgbm/_build/results?buildId=508&view=logs&j=9f61088a-36ee-57e4-0947-03329fd6ee90&t=9f61088a-36ee-57e4-0947-03329fd6ee90

^ don't worry about that build failing. I added this to .ci/test_r_package_windows.ps1 just to get meaningful install logs and then not spend CI time on tests

Rscript build_r.R
Check-Output $false

[StrikerRUS]

Thanks!
But I don't think that this PR should be marked as [R-package]. R-package allowed only to spot the warning due to compiler version.

[jameslamb]

Thanks!
But I don't think that this PR should be marked as [R-package]. R-package allowed only to spot the warning due to compiler version.

I think it is R-package specific. I think that the warning is only spotted because Log::Fatal ends in calling Rf_error for the R package.

This warning doesn't show up for non-R builds with MSVC, e.g. the sdist build we do for Python (https://dev.azure.com/lightgbm-ci/lightgbm-ci/_build/results?buildId=6111&view=logs&j=67ae2ff8-4b33-578b-0813-9c0440091667&t=130bfb73-8505-5e4f-f030-9ec1bace59ea)

There are no warnings around prediction_early_stop.cpp in the picture below:

[StrikerRUS]

This warning doesn't show up for non-R builds with MSVC, e.g. the sdist build we do for Python

We use VS 2017 there, but your screenshot from the original message shows that you were using VS 2019. Also please note that the same warnings fixed in #2866 were spotted only in Debug mode. So, I believe, Log::Fatal as a terminate command indeed causes those warnings but only under some conditions. I think it can be easily checked by running VS 2019 in Debug mode.

[jameslamb]

This warning doesn't show up for non-R builds with MSVC, e.g. the sdist build we do for Python

We use VS 2017 there, but your screenshot from the original message shows that you were using VS 2019. Also please note that the same warnings fixed in #2866 were spotted only in Debug mode. So, I believe, Log::Fatal as a terminate command indeed causes those warnings but only under some conditions. I think it can be easily checked by running VS 2019 in Debug mode.

ohhhhhhh ok I see! I'm fine to leave [R-package] off the description then

[jameslamb]

Adding that I just got this same warning on my Mac with clang++ tonight:

[jameslamb]

Adding that now I see this warning with MinGW g++ from Rtools

This has become an R CMD check warning for the CRAN package, so now I think this PR is officially important for #629 , not just minor maintenance to remove a warning that might distract users.

[jameslamb]

gently pinging C++ reviewers for a review. I think this one should be quick 👀

[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.