const'ness (constivity?) in sugar::min/max

[dcdillon]

The sugar::min and sugar::max classes (which to my mind should just be functions anyhow, but leaving that) have a cast operator to the typedef'ed STORAGE type. This operator is not defined as const because it actually modifies some private member variables. This is causing a problem with the latest fix for operator ambiguity in Matrix or Vector arithmetic operators.

I see no problem in moving these private member variables simply into the cast operator function and declaring the function as const. Does anyone else see any pitfalls?

I would propose changing:

template <int RTYPE, bool NA, typename T>
class Max {
public:
    typedef typename Rcpp::traits::storage_type<RTYPE>::type STORAGE ;

    Max( const T& obj_) : obj(obj_) {}

    operator STORAGE() {
        max_ = obj[0] ;
        if( Rcpp::traits::is_na<RTYPE>( max_ ) ) return max_ ;

        R_xlen_t n = obj.size() ;
        for( R_xlen_t i=1; i<n; i++){
            current = obj[i] ;
            if( Rcpp::traits::is_na<RTYPE>( current ) ) return current;
            if( current > max_ ) max_ = current ;
        }
        return max_ ;
    }

private:
    const T& obj ;
    STORAGE min_, max_, current ;
} ;

to

template <int RTYPE, bool NA, typename T>
class Max {
public:
    typedef typename Rcpp::traits::storage_type<RTYPE>::type STORAGE ;

    Max( const T& obj_) : obj(obj_) {}

    operator STORAGE() const {
        STORAGE min_, max_, current ;
        max_ = obj[0] ;
        if( Rcpp::traits::is_na<RTYPE>( max_ ) ) return max_ ;

        R_xlen_t n = obj.size() ;
        for( R_xlen_t i=1; i<n; i++){
            current = obj[i] ;
            if( Rcpp::traits::is_na<RTYPE>( current ) ) return current;
            if( current > max_ ) max_ = current ;
        }
        return max_ ;
    }

private:
    const T& obj ;
} ;

Alternatively, we could greatly simplify min/max by simply defining:

namespace Rcpp
{

template< int RTYPE, typename T >
typename traits::storage_type< RTYPE >::type max(const VectorBase< RTYPE, true, T > &x) {
    typedef typename Rcpp::traits::storage_type<RTYPE>::type STORAGE;

    STORAGE max, current;

    max = x[0];

    if (traits::is_na<RTYPE>(max)) return max;

    R_xlen_t n = x.size();

    for (R_xlen_t i = 1; i < n; ++i)
    {
        current = x[i];
        if (traits::is_na<RTYPE>(current)) return current;
        if (current > max) max = current;
    }

    return max;
}

template< int RTYPE, typename T >
typename traits::storage_type< RTYPE >::type max(const VectorBase< RTYPE, false, T > &x) {
    typedef typename Rcpp::traits::storage_type<RTYPE>::type STORAGE;

    STORAGE max, current;

    max = x[0];

    R_xlen_t n = x.size();

    for (R_xlen_t i = 1; i < n; ++i)
    {
        current = x[i];
        if (current > max) max = current;
    }

    return max;
}

} // namespace Rcpp

[eddelbuettel]

So the reverse dependency check threw out two new failures:

• CARBayesST balks on constness in max
• nonlinearTseries balks on constness in min

I tried applying the first suggested changeset here it didn't succeed.

[dcdillon]

diff --git a/inst/include/Rcpp/sugar/functions/max.h b/inst/include/Rcpp/sugar/functions/max.h
index 41d5de5..39ba5e7 100644
--- a/inst/include/Rcpp/sugar/functions/max.h
+++ b/inst/include/Rcpp/sugar/functions/max.h
@@ -45,6 +45,20 @@ namespace sugar{
             return max_ ;
         }

+        operator STORAGE() const {
+            STORAGE max, current ;
+            max = obj[0] ;
+            if( Rcpp::traits::is_na<RTYPE>( max_ ) ) return max_ ;
+
+            R_xlen_t n = obj.size() ;
+            for( R_xlen_t i=1; i<n; i++){
+                current = obj[i] ;
+                if( Rcpp::traits::is_na<RTYPE>( current ) ) return current;
+                if( current > max ) max = current ;
+            }
+            return max ;
+        }
+
     private:
         const T& obj ;
         STORAGE min_, max_, current ;
@@ -69,6 +83,18 @@ namespace sugar{
             return max_ ;
         }

+        operator STORAGE() const {
+            STORAGE max, current ;
+            max = obj[0] ;
+
+            R_xlen_t n = obj.size() ;
+            for( R_xlen_t i=1; i<n; i++){
+                current = obj[i] ;
+                if( current > max ) max = current ;
+            }
+            return max ;
+        }
+
     private:
         const T& obj ;
         STORAGE max_, current ;

[dcdillon]

Analogous diff for min

diff --git a/inst/include/Rcpp/sugar/functions/min.h b/inst/include/Rcpp/sugar/functions/min.h
index 0dd8c2c..3e83a76 100644
--- a/inst/include/Rcpp/sugar/functions/min.h
+++ b/inst/include/Rcpp/sugar/functions/min.h
@@ -45,6 +45,20 @@ namespace sugar{
             return min_ ;
         }

+        operator STORAGE() const {
+            STORAGE min, current ;
+            min = obj[0] ;
+            if( Rcpp::traits::is_na<RTYPE>( min ) ) return min ;
+
+            R_xlen_t n = obj.size() ;
+            for( R_xlen_t i=1; i<n; i++){
+                current = obj[i] ;
+                if( Rcpp::traits::is_na<RTYPE>( current ) ) return current;
+                if( current < min ) min = current ;
+            }
+            return min ;
+        }
+
         const T& obj ;
         STORAGE min_, max_, current ;
     } ;
@@ -68,6 +82,18 @@ namespace sugar{
             return min_ ;
         }

+        operator STORAGE() const {
+            STORAGE min, current ;
+            min = obj[0] ;
+
+            R_xlen_t n = obj.size() ;
+            for( R_xlen_t i=1; i<n; i++){
+                current = obj[i] ;
+                if( current < min ) min = current ;
+            }
+            return min ;
+        }
+
         const T& obj ;
         STORAGE min_, current ;
     } ;

[eddelbuettel]

Thanks. Had hand-cobbled diff for min in by now too.

Both CARBayesST and nonlinearTseries now pass. All good.

I guess ... you should wrap this up as a new PR.

[dcdillon]

I can do that...do you want the "extra complicated V1" as evidenced in the diffs?

[eddelbuettel]

No strong feeling either way. If we have the const variants now not using the private member variables, maybe we should clean up the non-const versions?

[dcdillon]

I think so...I'll do that. It's a trivial change.

[eddelbuettel]

Done in #478