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

replace built-in assert with own CHECK macro #2938

Merged
merged 2 commits into from
Apr 3, 2020
Merged

replace built-in assert with own CHECK macro #2938

merged 2 commits into from
Apr 3, 2020

Conversation

StrikerRUS
Copy link
Collaborator

It removes the difference between R-specific library and ordinary one.

Refer to #2837.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey great idea! Why is LightGBM::Log required for this?

@StrikerRUS
Copy link
Collaborator Author

@jameslamb

Why is LightGBM::Log required for this?

Because Json11 uses its own namespace

namespace json11 {

and Log is defined in LightGBM namespace

namespace LightGBM {

Refer to https://stackoverflow.com/a/11791202.

@jameslamb
Copy link
Collaborator

@jameslamb

Why is LightGBM::Log required for this?

Because Json11 uses its own namespace

namespace json11 {

and Log is defined in LightGBM namespace

namespace LightGBM {

Refer to https://stackoverflow.com/a/11791202.

oooooooh wild, ok! thanks for the explanation

@jameslamb jameslamb self-requested a review March 24, 2020 01:40
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I can see in the logs that this did not introduce any new R CMD CHECK issues.

@jameslamb
Copy link
Collaborator

@chivee or @guolinke could you take a look and let us know if you are ok with this?

Thanks!

@StrikerRUS StrikerRUS merged commit cc6a2f5 into master Apr 3, 2020
@StrikerRUS StrikerRUS deleted the assert branch April 3, 2020 13:32
@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