mutate no longer 🔥 when setting the first variable to NULL. closes #…

…1329
tidyverse · Aug 19, 2015 · 7fd36ef · 7fd36ef
1 parent 1b36d85
commit 7fd36ef
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 33 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -78,7 +78,9 @@
 
 * `mutate.rowwise_df` handles factors (#886).
 
-* `filter` works with rowwise data (#1099). 
+* `filter` works with rowwise data (#1099).
+
+* `mutate` can set to `NULL` the first column (used to segfault, #1329).  
 
 # dplyr 0.4.2
 

diff --git a/inst/include/tools/SymbolMap.h b/inst/include/tools/SymbolMap.h
@@ -2,22 +2,22 @@
 #define dplyr_tools_SymbolMap_h
 
 namespace dplyr{
-   
+
     enum Origin { HASH, RMATCH, NEW } ;
-            
+
     struct SymbolMapIndex {
         int pos ;
         Origin origin ;
-        
-        SymbolMapIndex( int pos_, Origin origin_ ) : 
+
+        SymbolMapIndex( int pos_, Origin origin_ ) :
             pos(pos_), origin(origin_)
         {}
     } ;
-    
+
     class SymbolMap {
         public:
             SymbolMap(): lookup(), r_match( "match" ), names(){}
-            
+
             SymbolMapIndex insert( SEXP name ){
                 if( TYPEOF(name) == SYMSXP ) {
                     name = PRINTNAME(name) ;
@@ -37,52 +37,52 @@ namespace dplyr{
                 } ;
                 return index ;
             }
-            
+
             int size() const {
-                return names.size() ;    
+                return names.size() ;
             }
-            
+
             bool has( SEXP name ) const {
                 if( TYPEOF(name) == SYMSXP ) {
                     name = PRINTNAME(name) ;
                 }
                 SymbolMapIndex index = get_index(name) ;
                 return index.origin != NEW ;
             }
-            
+
             SymbolMapIndex get_index(SEXP name) const {
                 if( TYPEOF(name) == SYMSXP ) {
                     name = PRINTNAME(name) ;
                 }
-                
+
                 // first, lookup the map
                 dplyr_hash_map<SEXP, int>::const_iterator it = lookup.find(name) ;
                 if( it != lookup.end() ){
-                    return SymbolMapIndex( it->second, HASH ) ;    
+                    return SymbolMapIndex( it->second, HASH ) ;
                 }
-                
+
                 CharacterVector v = CharacterVector::create(name) ;
                 int idx = as<int>( r_match( v, names ) );
                 if( idx != NA_INTEGER ){
                     // we have a match
                     return SymbolMapIndex( idx-1, RMATCH ) ;
                 }
-                
+
                 // no match
                 return SymbolMapIndex( names.size(), NEW ) ;
             }
-            
+
             int get( SEXP name ) const {
                 if( TYPEOF(name) == SYMSXP ) {
                     name = PRINTNAME(name) ;
                 }
                 SymbolMapIndex index = get_index(name) ;
                 if( index.origin == NEW ){
-                    stop( "variable '%s' not found", CHAR(name) ) ;    
+                    stop( "variable '%s' not found", CHAR(name) ) ;
                 }
                 return index.pos ;
             }
-            
+
             SymbolMapIndex rm( SEXP name ){
                 if( TYPEOF(name) == SYMSXP ) {
                     name = PRINTNAME(name) ;
@@ -91,36 +91,37 @@ namespace dplyr{
                 if( index.origin != NEW ){
                     int idx = index.pos ;
                     names.erase( names.begin() + idx ) ;
-                    
-                    for( dplyr_hash_map<SEXP, int>::iterator it=lookup.begin(); it != lookup.end(); ++it){
+
+                    for( dplyr_hash_map<SEXP, int>::iterator it=lookup.begin(); it != lookup.end(); ){
                         int k = it->second ;
 
-                        // nothing to do in that case
-                        if( k < idx ) continue ;
-
-                        if( k == idx ){
+                        if( k < idx ) {
+                          // nothing to do in that case
+                          ++it ;
+                          continue ;
+                        } else if( k == idx ){
                             // need to remove the data from the hash table
                             it = lookup.erase(it) ;
                             continue ;
                         } else {
                             // decrement the index
-                            it->second-- ;    
+                            it->second-- ;
+                            ++it ;
                         }
-
                     }
-                    
+
                 }
-                
+
                 return index ;
             }
-            
-            
+
+
             dplyr_hash_map<SEXP, int> lookup ;
-            Function r_match ;  
+            Function r_match ;
             CharacterVector names ;
-                        
+
     } ;
-    
+
 } ;
 
 #endif
diff --git a/tests/testthat/test-mutate.r b/tests/testthat/test-mutate.r
@@ -402,3 +402,12 @@ test_that("mutate.rowwise handles factors (#886)", {
     mutate(processed_trafo=paste("test", processed))
   expect_equal( res$processed_trafo, c("test foo", "test bar"))
 })
+
+test_that("setting first column to NULL with mutate works (#1329)", {
+    df <- data.frame(x = 1:10, y = 1:10)
+    expect_equal( mutate(df, x=NULL), select(df,-x) )
+    expect_equal( mutate(df, y=NULL), select(df,-y) )
+
+    gdf <- group_by(df, y)
+    expect_equal( select(gdf, -x), mutate(gdf, x = NULL) )
+})