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

ROBUSTNESS: x & y should be x && y #434

Closed
HenrikBengtsson opened this issue Jun 2, 2021 · 4 comments
Closed

ROBUSTNESS: x & y should be x && y #434

HenrikBengtsson opened this issue Jun 2, 2021 · 4 comments
Assignees
Labels
Enhancement Not actually a bug but a possible improvement

Comments

@HenrikBengtsson
Copy link

Hi, while troubleshooting some future devel issues regarding lidR, I spotted:

https://github.com/Jean-Romain/lidR/blob/82ef90be73cd3e8e86a49e8cb0d920e1fa751927/R/clusters_apply.R#L129
https://github.com/Jean-Romain/lidR/blob/82ef90be73cd3e8e86a49e8cb0d920e1fa751927/R/clusters_apply.R#L147

I think you meant to use && (scalar) instead of & (vector).

@Jean-Romain
Copy link
Collaborator

Jean-Romain commented Jun 2, 2021

I can do it. I do agree that programmatically speaking it is more correct that way but I can't see how it might be a problem. & and && are similar enough with length 1 vector for the two expressions with & or && to behave almost the same.

Actually in R I always use & unless I explicitly want the second term not being evaluated if the first is FALSE like in !is.null(x) && x == 0

Jean-Romain added a commit that referenced this issue Jun 2, 2021
@HenrikBengtsson
Copy link
Author

I do agree that programmatically speaking it is more correct that way but I can't see how it might be a problem. & and && are similar enough with length 1 vector for the two expressions with & or && to behave almost the same.

Sorry, I should have been more clear. Technically, yes, both work, if and only if, LHS and RHS are of length one. However, if one make a mistake somewhere causing one of them to have length > 1, using && will eventually catch the mistake instead of propagating an unknown behavior. That's why I proposed it. See below for an example. When I run revdep checks on future, matrixStats, etc. with these checks enabled, I keep spotting quite a few number of packages with such bugs.

Current behavior in R:

$ R --quiet --vanilla 
> c(TRUE, FALSE) && TRUE
[1] TRUE
> 

Future/eventual behavior in R:

$ _R_CHECK_LENGTH_1_LOGIC2_=true R --quiet --vanilla
> c(TRUE, FALSE) && TRUE
Error in c(TRUE, FALSE) && TRUE : 
  'length(x) = 2 > 1' in coercion to 'logical(1)'

@HenrikBengtsson
Copy link
Author

To clarify further; I did not spot this mistake in lidR; I just happened to see the code while troubleshooting #435

@Jean-Romain
Copy link
Collaborator

Understood. Thanks. I'm trying to use && more consistently but & working every-time old habits are hard to shake off.

@Jean-Romain Jean-Romain self-assigned this Jun 2, 2021
@Jean-Romain Jean-Romain added the Enhancement Not actually a bug but a possible improvement label Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Not actually a bug but a possible improvement
Projects
None yet
Development

No branches or pull requests

2 participants