Final cleanup of io_data_t now that I recognize the bizarre way in which it shares output buffers (not sure how this ever worked before)

exec.cpp

@@ -420,7 +420,7 @@ static io_data_t *io_transmogrify( io_data_t * in )
 	if( !in )
 		return 0;
 	
-	std::auto_ptr<io_data_t> out(new io_data_t());
+	std::auto_ptr<io_data_t> out(new io_data_t);
 	
 	out->fd = in->fd;
 	out->io_mode = IO_FD;
@@ -437,7 +437,7 @@ static io_data_t *io_transmogrify( io_data_t * in )
 		case IO_BUFFER:
 		case IO_PIPE:
 		{
-            *out = *in;
+            out.reset(new io_data_t(*in));
 			break;
 		}
 
@@ -1039,7 +1039,10 @@ void exec( parser_t &parser, job_t *j )
 				
 				io_buffer_read( io_buffer );
 				
-				if( io_buffer->out_buffer->used != 0 )
+                const char *buffer = io_buffer->out_buffer_ptr();
+                size_t count = io_buffer->out_buffer_size();
+                
+				if( io_buffer->out_buffer_size() > 0 )
 				{
                     /* We don't have to drain threads here because our child process is simple */
 					pid = execute_fork(false);
@@ -1052,10 +1055,7 @@ void exec( parser_t &parser, job_t *j )
 						p->pid = getpid();
 						setup_child_process( j, p );
 
-						exec_write_and_exit(io_buffer->fd, 
-											io_buffer->out_buffer->buff,
-											io_buffer->out_buffer->used,
-											status);
+						exec_write_and_exit(io_buffer->fd, buffer, count, status);
 					}
 					else
 					{
@@ -1090,8 +1090,10 @@ void exec( parser_t &parser, job_t *j )
 			case INTERNAL_BUFFER:
 			{
 		
+				const char *buffer = io_buffer->out_buffer_ptr();
+				size_t count = io_buffer->out_buffer_size();
+        
 				pid = execute_fork(false);
-				
 				if( pid == 0 )
 				{
 					/*
@@ -1101,10 +1103,7 @@ void exec( parser_t &parser, job_t *j )
 					p->pid = getpid();
 					setup_child_process( j, p );
 					
-					exec_write_and_exit( 1,
-										 input_redirect->out_buffer->buff, 
-										 input_redirect->out_buffer->used,
-										 0);
+					exec_write_and_exit( 1, buffer, count, 0);
 				}
 				else
 				{
@@ -1157,7 +1156,7 @@ void exec( parser_t &parser, job_t *j )
 					( buffer_stdout ) )
 				{
 					std::string res = wcs2string( get_stdout_buffer() );
-					b_append( io->out_buffer, res.c_str(), res.size() );
+					io->out_buffer_append( res.c_str(), res.size() );
 					skip_fork = 1;
 				}
 
@@ -1378,9 +1377,9 @@ static int exec_subshell_internal( const wcstring &cmd, wcstring_list_t *lst )
 	
 	is_subshell = prev_subshell;
 	
-	b_append( io_buffer->out_buffer, &z, 1 );
+	io_buffer->out_buffer_append( &z, 1 );
 	
-	begin=end=io_buffer->out_buffer->buff;	
+	begin=end=io_buffer->out_buffer_ptr();	
 
 	if( lst )
 	{

history.h

@@ -14,7 +14,6 @@
 #include <list>
 #include <tr1/memory>
 #include <set>
-using std::tr1::shared_ptr;
 
 typedef std::list<wcstring> path_list_t;
 

io.cpp

@@ -91,7 +91,7 @@ void io_buffer_read( io_data_t *d )
 			}
 			else
 			{				
-				b_append( d->out_buffer, b, l );				
+				d->out_buffer_append( b, l );				
 			}
 		}
 	}
@@ -100,21 +100,18 @@ void io_buffer_read( io_data_t *d )
 
 io_data_t *io_buffer_create( int is_input )
 {
-	std::auto_ptr<io_data_t> buffer_redirect(new io_data_t());
-	
+	std::auto_ptr<io_data_t> buffer_redirect(new io_data_t);
+	buffer_redirect->out_buffer_create();
 	buffer_redirect->io_mode=IO_BUFFER;
 	buffer_redirect->next=0;
-	buffer_redirect->out_buffer= (buffer_t *)malloc( sizeof(buffer_t));
 	buffer_redirect->is_input = is_input;
-	b_init( buffer_redirect->out_buffer );
 	buffer_redirect->fd=is_input?0:1;
 	
 	if( exec_pipe( buffer_redirect->param1.pipe_fd ) == -1 )
 	{
 		debug( 1, PIPE_ERROR );
 		wperror (L"pipe");
-		free(  buffer_redirect->out_buffer );
-		return 0;
+		return NULL;
 	}
 	else if( fcntl( buffer_redirect->param1.pipe_fd[0],
 					F_SETFL,
@@ -122,8 +119,7 @@ io_data_t *io_buffer_create( int is_input )
 	{
 		debug( 1, PIPE_ERROR );
 		wperror( L"fcntl" );
-		free(  buffer_redirect->out_buffer );
-		return 0;
+		return NULL;
 	}
 	return buffer_redirect.release();
 }
@@ -146,10 +142,6 @@ void io_buffer_destroy( io_data_t *io_buffer )
 	  Dont free fd for writing. This should already be free'd before
 	  calling exec_read_io_buffer on the buffer
 	*/
-	
-	b_destroy( io_buffer->out_buffer );
-	
-	free( io_buffer->out_buffer );
 	delete io_buffer;
 }
 

io.h

@@ -1,22 +1,33 @@
 #ifndef FISH_IO_H
 #define FISH_IO_H
 
+#include <tr1/memory>
+using std::tr1::shared_ptr;
+
 /**
    Describes what type of IO operation an io_data_t represents
 */
 enum io_mode
 {
 	IO_FILE, IO_PIPE, IO_FD, IO_BUFFER, IO_CLOSE
-}
-;
+};
 
 /** Represents an FD redirection */
-struct io_data_t
+class io_data_t
 {
+private:
+    /** buffer to save output in for IO_BUFFER. Note that in the original fish, the buffer was a pointer to a buffer_t stored in the param2 union down below, and when an io_data_t was duplicated the pointer was copied so that two io_data_ts referenced the same buffer. It's not clear to me how this was ever cleaned up correctly. But it's important that they share the same buffer for reasons I don't yet understand either. But we can get correct sharing and cleanup with shared_ptr. */
+    shared_ptr<std::vector<char> > out_buffer;
+
+    /* No assignment allowed */
+    void operator=(const io_data_t &rhs) { assert(0); }
+
+public:
 	/** Type of redirect */
 	int io_mode;
 	/** FD to redirect */
 	int fd;
+
 	/** 
 		Type-specific parameter for redirection 
 	*/
@@ -31,9 +42,7 @@ struct io_data_t
     /** Filename IO_FILE */
     wcstring filename;
 
-	/** 
-		Second type-specific paramter for redirection
-	*/
+	/**  Second type-specific paramter for redirection */
 	union
 	{
 		/** file creation flags to send to open for IO_FILE */
@@ -42,8 +51,28 @@ struct io_data_t
 		int close_old;
 	} param2;
     
-    /** buffer to save output in for IO_BUFFER */
-    buffer_t *out_buffer;		
+    /** Function to create the output buffer */
+    void out_buffer_create() {
+        out_buffer.reset(new std::vector<char>);
+    }
+        
+    /** Function to append to the buffer */
+    void out_buffer_append(const char *ptr, size_t count) {
+        assert(out_buffer.get() != NULL); 
+        out_buffer->insert(out_buffer->end(), ptr, ptr + count);
+    }
+    
+    /** Function to get a pointer to the buffer */
+    char *out_buffer_ptr(void) {
+        assert(out_buffer.get() != NULL);
+        return &out_buffer->at(0);
+    }
+    
+    /** Function to get the size of the buffer */
+    size_t out_buffer_size(void) const {
+        assert(out_buffer.get() != NULL);
+        return out_buffer->size();
+    }
 
 	/** Set to true if this is an input io redirection */
 	int is_input;
@@ -51,7 +80,11 @@ struct io_data_t
 	/** Pointer to the next IO redirection */
 	io_data_t *next;
     
-    io_data_t() : next(NULL) { }
+    io_data_t() : next(NULL)
+    {
+    }
+    
+    /* Note: we have a default copy constructor */
 };
 
  

postfork.cpp

@@ -352,4 +352,5 @@ pid_t execute_fork(bool wait_for_threads_to_die)
 	debug( 0, FORK_ERROR );
 	wperror (L"fork");
 	FATAL_EXIT();
+    return 0;
 }

proc.cpp

@@ -881,7 +881,7 @@ static void read_try( job_t *j )
 			}
 			else
 			{
-				b_append( buff->out_buffer, b, l );
+                buff->out_buffer_append(b, l);
 			}			
 		}
 	}

reader.cpp

@@ -1174,7 +1174,7 @@ static void run_pager( wchar_t *prefix, int is_quoted, const std::vector<complet
 	free( escaped_separator );		
 	
 	foo = wcs2str(msg.c_str());
-	b_append( in->out_buffer, foo, strlen(foo) );
+	in->out_buffer_append(foo, strlen(foo) );
 	free( foo );
 	
 	term_donate();
@@ -1190,10 +1190,10 @@ static void run_pager( wchar_t *prefix, int is_quoted, const std::vector<complet
 	io_buffer_read( out );
 
 	int nil=0;
-	b_append( out->out_buffer, &nil, 1 );
+    out->out_buffer_append((char *)&nil, 1);
 
 	wchar_t *tmp;
-	wchar_t *str = str2wcs((char *)out->out_buffer->buff);
+	wchar_t *str = str2wcs(out->out_buffer_ptr());
 
 	if( str )
 	{

wutil.cpp

@@ -66,8 +66,6 @@ void wutil_destroy()
 {
 }
 
-static pthread_mutex_t readdir_lock = PTHREAD_MUTEX_INITIALIZER;
-
 bool wreaddir_resolving(DIR *dir, const std::wstring &dir_path, std::wstring &out_name, bool *out_is_dir)
 {
     struct dirent *d = readdir( dir );