Skip to content

Commit dcca2a1

Browse files
kevinusheykevinushey
committed
fix !, &&, and || on single logical results
None of `!all(x)`, `all(x) && any(y)`, `all(x) || any(y)`, `all(x) && true` or `all(x) || true` compiled: the combinators called the non-const get() through const references, the && and || overloads for two results declared their second parameter with the first one's template arguments (so they could never be deduced), and Or_SingleLogicalResult_bool named the And class as its CRTP base. The combinators now hold their operands by value, like other sugar expressions, and the general && and || follow R's three-valued logic (NA && FALSE is FALSE, NA || TRUE is TRUE).
1 parent 3292c48 commit dcca2a1

8 files changed

Lines changed: 164 additions & 41 deletions

File tree

‎ChangeLog‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,14 @@
1414
* inst/include/Rcpp/stats/dpq/dpq.h: Idem
1515
* inst/include/Rcpp/sugar/block/SugarBlock_3.h: Also fix the type of the
1616
third operand
17+
* inst/include/Rcpp/sugar/logical/and.h: Fix operator&& on two single
18+
logical results, hold their operands by value, and make NA && FALSE
19+
return FALSE
20+
* inst/include/Rcpp/sugar/logical/or.h: Idem for operator||, making
21+
NA || TRUE return TRUE, and fix the base class of Or_SingleLogicalResult_bool
22+
* inst/include/Rcpp/sugar/logical/not.h: Hold the operand of operator!
23+
by value
24+
* inst/include/Rcpp/sugar/logical/SingleLogicalResult.h: Add get_ref()
1725
* inst/include/Rcpp/vector/Vector.h: Evaluate sugar expressions that
1826
aren't elementwise before assigning them in place
1927
* inst/include/Rcpp/vector/RangeIndexer.h: Idem, and check lengths

‎inst/NEWS.Rd‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@
3131
stored, evaluate expressions that may read the target before assigning
3232
them in place, and signal an error for operands of different lengths
3333
(Kevin in \ghpr{1513} closing \ghit{1512})
34+
\item The sugar operators \code{!}, \code{&&} and \code{||} on single
35+
logical results such as \code{all()} compile again, and follow R's
36+
handling of \code{NA} (Kevin in \ghpr{1513})
3437
}
3538
\item Changes in Rcpp Documentation:
3639
\itemize{

‎inst/include/Rcpp/sugar/logical/SingleLogicalResult.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,14 @@ class SingleLogicalResult {
4545

4646
SingleLogicalResult() : result(UNRESOLVED) {} ;
4747

48+
T& get_ref(){
49+
return static_cast<T&>(*this) ;
50+
}
51+
52+
const T& get_ref() const {
53+
return static_cast<const T&>(*this) ;
54+
}
55+
4856
void apply(){
4957
if( result == UNRESOLVED ){
5058
static_cast<T&>(*this).apply() ;

‎inst/include/Rcpp/sugar/logical/and.h‎

Lines changed: 26 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -41,22 +41,29 @@ public SingleLogicalResult<
4141
> BASE ;
4242

4343
And_SingleLogicalResult_SingleLogicalResult( const LHS_TYPE& lhs_, const RHS_TYPE& rhs_) :
44-
lhs(lhs_), rhs(rhs_){} ;
44+
lhs(lhs_.get_ref()), rhs(rhs_.get_ref()){} ;
4545

4646
inline void apply(){
4747
int left = lhs.get() ;
48-
if( Rcpp::traits::is_na<LGLSXP>( left ) ){
49-
BASE::set( left ) ;
50-
} else if( left == FALSE ){
48+
if( left == FALSE ){
5149
BASE::set( FALSE ) ;
50+
return ;
51+
}
52+
53+
// NA && FALSE is FALSE
54+
int right = rhs.get() ;
55+
if( Rcpp::traits::is_na<LGLSXP>( left ) && right != FALSE ){
56+
BASE::set( left ) ;
5257
} else {
53-
BASE::set( rhs.get() ) ;
58+
BASE::set( right ) ;
5459
}
5560
}
5661

5762
private:
58-
const LHS_TYPE& lhs ;
59-
const RHS_TYPE& rhs ;
63+
// by value, since these are usually temporaries, and non-const, since
64+
// evaluating them caches their result
65+
LHS_T lhs ;
66+
RHS_T rhs ;
6067

6168
} ;
6269

@@ -77,7 +84,7 @@ public SingleLogicalResult<
7784
> BASE ;
7885

7986
And_SingleLogicalResult_SingleLogicalResult( const LHS_TYPE& lhs_, const RHS_TYPE& rhs_) :
80-
lhs(lhs_), rhs(rhs_){} ;
87+
lhs(lhs_.get_ref()), rhs(rhs_.get_ref()){} ;
8188

8289
inline void apply(){
8390
// here we know rhs does not have NA, so we start with the rhs
@@ -90,8 +97,8 @@ public SingleLogicalResult<
9097
}
9198

9299
private:
93-
const LHS_TYPE& lhs ;
94-
const RHS_TYPE& rhs ;
100+
LHS_T lhs ;
101+
RHS_T rhs ;
95102

96103
} ;
97104

@@ -113,7 +120,7 @@ public SingleLogicalResult<
113120
> BASE ;
114121

115122
And_SingleLogicalResult_SingleLogicalResult( const LHS_TYPE& lhs_, const RHS_TYPE& rhs_) :
116-
lhs(lhs_), rhs(rhs_){} ;
123+
lhs(lhs_.get_ref()), rhs(rhs_.get_ref()){} ;
117124

118125
inline void apply(){
119126
// here we know lhs does not have NA, so we start with the rhs
@@ -126,8 +133,8 @@ public SingleLogicalResult<
126133
}
127134

128135
private:
129-
const LHS_TYPE& lhs ;
130-
const RHS_TYPE& rhs ;
136+
LHS_T lhs ;
137+
RHS_T rhs ;
131138

132139
} ;
133140

@@ -148,7 +155,7 @@ public SingleLogicalResult<
148155
> BASE ;
149156

150157
And_SingleLogicalResult_SingleLogicalResult( const LHS_TYPE& lhs_, const RHS_TYPE& rhs_) :
151-
lhs(lhs_), rhs(rhs_){} ;
158+
lhs(lhs_.get_ref()), rhs(rhs_.get_ref()){} ;
152159

153160
inline void apply(){
154161
int left = lhs.get() ;
@@ -160,8 +167,8 @@ public SingleLogicalResult<
160167
}
161168

162169
private:
163-
const LHS_TYPE& lhs ;
164-
const RHS_TYPE& rhs ;
170+
LHS_T lhs ;
171+
RHS_T rhs ;
165172

166173
} ;
167174

@@ -182,7 +189,7 @@ public SingleLogicalResult<
182189
> BASE ;
183190

184191
And_SingleLogicalResult_bool( const LHS_TYPE& lhs_, bool rhs_) :
185-
lhs(lhs_), rhs(rhs_){} ;
192+
lhs(lhs_.get_ref()), rhs(rhs_){} ;
186193

187194
inline void apply(){
188195
if( !rhs ){
@@ -193,7 +200,7 @@ public SingleLogicalResult<
193200
}
194201

195202
private:
196-
const LHS_TYPE& lhs ;
203+
LHS_T lhs ;
197204
bool rhs ;
198205

199206
} ;
@@ -299,7 +306,7 @@ template <bool LHS_NA, typename LHS_T, bool RHS_NA, typename RHS_T>
299306
inline Rcpp::sugar::And_SingleLogicalResult_SingleLogicalResult<LHS_NA,LHS_T,RHS_NA,RHS_T>
300307
operator&&(
301308
const Rcpp::sugar::SingleLogicalResult<LHS_NA,LHS_T>& lhs,
302-
const Rcpp::sugar::SingleLogicalResult<LHS_NA,LHS_T>& rhs
309+
const Rcpp::sugar::SingleLogicalResult<RHS_NA,RHS_T>& rhs
303310
){
304311
return Rcpp::sugar::And_SingleLogicalResult_SingleLogicalResult<LHS_NA,LHS_T,RHS_NA,RHS_T>( lhs, rhs ) ;
305312
}

‎inst/include/Rcpp/sugar/logical/not.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,14 +43,16 @@ class Negate_SingleLogicalResult : public SingleLogicalResult<NA, Negate_SingleL
4343
public:
4444
typedef SingleLogicalResult<NA,T> TYPE ;
4545
typedef SingleLogicalResult<NA, Negate_SingleLogicalResult<NA,T> > BASE ;
46-
Negate_SingleLogicalResult( const TYPE& orig_ ) : orig(orig_) {}
46+
Negate_SingleLogicalResult( const TYPE& orig_ ) : orig(orig_.get_ref()) {}
4747

4848
inline void apply(){
4949
BASE::set( negate<NA>::apply( orig.get() ) );
5050
}
5151

5252
private:
53-
const TYPE& orig ;
53+
// by value, since it is usually a temporary, and non-const, since
54+
// evaluating it caches its result
55+
T orig ;
5456

5557
} ;
5658

‎inst/include/Rcpp/sugar/logical/or.h‎

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -41,22 +41,29 @@ public SingleLogicalResult<
4141
> BASE ;
4242

4343
Or_SingleLogicalResult_SingleLogicalResult( const LHS_TYPE& lhs_, const RHS_TYPE& rhs_) :
44-
lhs(lhs_), rhs(rhs_){} ;
44+
lhs(lhs_.get_ref()), rhs(rhs_.get_ref()){} ;
4545

4646
inline void apply(){
4747
int left = lhs.get() ;
48-
if( Rcpp::traits::is_na<LGLSXP>( left ) ){
49-
BASE::set( left ) ;
50-
} else if( left == TRUE ){
48+
if( left == TRUE ){
5149
BASE::set( TRUE ) ;
50+
return ;
51+
}
52+
53+
// NA || TRUE is TRUE
54+
int right = rhs.get() ;
55+
if( Rcpp::traits::is_na<LGLSXP>( left ) && right != TRUE ){
56+
BASE::set( left ) ;
5257
} else {
53-
BASE::set( rhs.get() ) ;
58+
BASE::set( right ) ;
5459
}
5560
}
5661

5762
private:
58-
const LHS_TYPE& lhs ;
59-
const RHS_TYPE& rhs ;
63+
// by value, since these are usually temporaries, and non-const, since
64+
// evaluating them caches their result
65+
LHS_T lhs ;
66+
RHS_T rhs ;
6067

6168
} ;
6269

@@ -77,7 +84,7 @@ public SingleLogicalResult<
7784
> BASE ;
7885

7986
Or_SingleLogicalResult_SingleLogicalResult( const LHS_TYPE& lhs_, const RHS_TYPE& rhs_) :
80-
lhs(lhs_), rhs(rhs_){} ;
87+
lhs(lhs_.get_ref()), rhs(rhs_.get_ref()){} ;
8188

8289
inline void apply(){
8390
// here we know rhs does not have NA, so we start with the rhs
@@ -90,8 +97,8 @@ public SingleLogicalResult<
9097
}
9198

9299
private:
93-
const LHS_TYPE& lhs ;
94-
const RHS_TYPE& rhs ;
100+
LHS_T lhs ;
101+
RHS_T rhs ;
95102

96103
} ;
97104

@@ -113,7 +120,7 @@ public SingleLogicalResult<
113120
> BASE ;
114121

115122
Or_SingleLogicalResult_SingleLogicalResult( const LHS_TYPE& lhs_, const RHS_TYPE& rhs_) :
116-
lhs(lhs_), rhs(rhs_){} ;
123+
lhs(lhs_.get_ref()), rhs(rhs_.get_ref()){} ;
117124

118125
inline void apply(){
119126
// here we know lhs does not have NA, so we start with the rhs
@@ -126,8 +133,8 @@ public SingleLogicalResult<
126133
}
127134

128135
private:
129-
const LHS_TYPE& lhs ;
130-
const RHS_TYPE& rhs ;
136+
LHS_T lhs ;
137+
RHS_T rhs ;
131138

132139
} ;
133140

@@ -148,7 +155,7 @@ public SingleLogicalResult<
148155
> BASE ;
149156

150157
Or_SingleLogicalResult_SingleLogicalResult( const LHS_TYPE& lhs_, const RHS_TYPE& rhs_) :
151-
lhs(lhs_), rhs(rhs_){} ;
158+
lhs(lhs_.get_ref()), rhs(rhs_.get_ref()){} ;
152159

153160
inline void apply(){
154161
int left = lhs.get() ;
@@ -160,8 +167,8 @@ public SingleLogicalResult<
160167
}
161168

162169
private:
163-
const LHS_TYPE& lhs ;
164-
const RHS_TYPE& rhs ;
170+
LHS_T lhs ;
171+
RHS_T rhs ;
165172

166173
} ;
167174

@@ -170,7 +177,7 @@ template <bool LHS_NA, typename LHS_T>
170177
class Or_SingleLogicalResult_bool :
171178
public SingleLogicalResult<
172179
LHS_NA ,
173-
And_SingleLogicalResult_bool<LHS_NA,LHS_T>
180+
Or_SingleLogicalResult_bool<LHS_NA,LHS_T>
174181
>
175182
{
176183
public:
@@ -181,7 +188,7 @@ public SingleLogicalResult<
181188
> BASE ;
182189

183190
Or_SingleLogicalResult_bool( const LHS_TYPE& lhs_, bool rhs_) :
184-
lhs(lhs_), rhs(rhs_){} ;
191+
lhs(lhs_.get_ref()), rhs(rhs_){} ;
185192

186193
inline void apply(){
187194
if( rhs ){
@@ -192,7 +199,7 @@ public SingleLogicalResult<
192199
}
193200

194201
private:
195-
const LHS_TYPE& lhs ;
202+
LHS_T lhs ;
196203
bool rhs ;
197204

198205
} ;
@@ -298,7 +305,7 @@ template <bool LHS_NA, typename LHS_T, bool RHS_NA, typename RHS_T>
298305
inline Rcpp::sugar::Or_SingleLogicalResult_SingleLogicalResult<LHS_NA,LHS_T,RHS_NA,RHS_T>
299306
operator||(
300307
const Rcpp::sugar::SingleLogicalResult<LHS_NA,LHS_T>& lhs,
301-
const Rcpp::sugar::SingleLogicalResult<LHS_NA,LHS_T>& rhs
308+
const Rcpp::sugar::SingleLogicalResult<RHS_NA,RHS_T>& rhs
302309
){
303310
return Rcpp::sugar::Or_SingleLogicalResult_SingleLogicalResult<LHS_NA,LHS_T,RHS_NA,RHS_T>( lhs, rhs ) ;
304311
}

‎inst/tinytest/cpp/sugar_expressions.cpp‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,3 +288,65 @@ NumericMatrix length_row(NumericMatrix m, NumericVector y) {
288288
m(0, _) = y;
289289
return m;
290290
}
291+
292+
// [[Rcpp::export]]
293+
SEXP single_and(LogicalVector a, LogicalVector b) {
294+
return all(a) && all(b);
295+
}
296+
297+
// [[Rcpp::export]]
298+
SEXP single_or(LogicalVector a, LogicalVector b) {
299+
return all(a) || all(b);
300+
}
301+
302+
// [[Rcpp::export]]
303+
SEXP single_not(LogicalVector a) {
304+
return !all(a);
305+
}
306+
307+
// [[Rcpp::export]]
308+
SEXP single_and_nona_lhs(LogicalVector a, LogicalVector b) {
309+
return all(noNA(a)) && all(b);
310+
}
311+
312+
// [[Rcpp::export]]
313+
SEXP single_and_nona_rhs(LogicalVector a, LogicalVector b) {
314+
return all(a) && all(noNA(b));
315+
}
316+
317+
// [[Rcpp::export]]
318+
SEXP single_and_nona_both(LogicalVector a, LogicalVector b) {
319+
return all(noNA(a)) && all(noNA(b));
320+
}
321+
322+
// [[Rcpp::export]]
323+
SEXP single_or_nona_lhs(LogicalVector a, LogicalVector b) {
324+
return all(noNA(a)) || all(b);
325+
}
326+
327+
// [[Rcpp::export]]
328+
SEXP single_or_nona_rhs(LogicalVector a, LogicalVector b) {
329+
return all(a) || all(noNA(b));
330+
}
331+
332+
// [[Rcpp::export]]
333+
SEXP single_or_nona_both(LogicalVector a, LogicalVector b) {
334+
return all(noNA(a)) || all(noNA(b));
335+
}
336+
337+
// [[Rcpp::export]]
338+
SEXP single_and_bool(LogicalVector a, bool b) {
339+
return all(a) && b;
340+
}
341+
342+
// [[Rcpp::export]]
343+
SEXP single_or_bool(LogicalVector a, bool b) {
344+
return b || all(a);
345+
}
346+
347+
// [[Rcpp::export]]
348+
SEXP single_stored(LogicalVector a, LogicalVector b) {
349+
auto e = !(all(a) && any(b));
350+
clobber_stack();
351+
return e;
352+
}

0 commit comments

Comments
 (0)