[svn-r15386] Purpose: Fix bug

Description:
    A bug in reference counter was exposed when Ray fixed H5Awrite in
    the main library.  ::setId() called incRefCount when it shouldn't.
    Made sure that id's reference counter is manually incremented
    properly in copy constructor and operator= only.  The main library
    handles the rest.

Platforms tested:
    SunOS 5.10 (linew)
    Linux 2.6 (kagiso)
    FreeBSD (duty)

9 files changed, 91 insertions, 188 deletions

diff --git a/c++/src/H5Attribute.cpp b/c++/src/H5Attribute.cpp
index 0182816..f85b87c 100644
--- a/c++/src/H5Attribute.cpp
+++ b/c++/src/H5Attribute.cpp
@@ -26,7 +26,6 @@
 #include "H5PropList.h"
 #include "H5Object.h"
 #include "H5AbstractDs.h"
-#include "H5Attribute.h"
 #include "H5FaccProp.h"
 #include "H5FcreatProp.h"
 #include "H5DcreatProp.h"
@@ -34,6 +33,8 @@
 #include "H5DataType.h"
 #include "H5DataSpace.h"
 #include "H5File.h"
+#include "H5Attribute.h"
+#include "H5private.h"          // for HDfree
 
 #ifndef H5_NO_NAMESPACE
 namespace H5 {
@@ -147,29 +148,46 @@ void Attribute::read( const DataType& mem_type, void *buf ) const
 //--------------------------------------------------------------------------
 void Attribute::read( const DataType& mem_type, H5std_string& strg ) const
 {
-   // Get the attribute size and allocate temporary C-string for C API
-   hsize_t attr_size = H5Aget_storage_size(id);
-   if (attr_size <= 0)
-   {
-      throw AttributeIException("Attribute::read", "Unable to get attribute size before reading");
-   }
-   char* strg_C = new char [attr_size+1];
-   if (strg_C == NULL)
-   {
-      throw AttributeIException("Attribute::read", "Unable to allocate buffer to read the attribute");
-   }
+    // Get the size of the attribute data.
+    hsize_t attr_size = H5Aget_storage_size(id);
+    if (attr_size <= 0)
+    {
+	throw AttributeIException("Attribute::read", "Unable to get attribute size before reading");
+    }
 
-   // Call C API to get the attribute data, a string of chars
-   herr_t ret_value = H5Aread(id, mem_type.getId(), &strg_C);
-   if( ret_value < 0 )
-   {
-      throw AttributeIException("Attribute::read", "H5Aread failed");
-   }
+    // Check if this attribute has variable-len string or fixed-len string and
+    // proceed appropriately.
+    bool is_variable_len = H5Tis_variable_str(mem_type.getId());
+    char *strg_C;
+    void *ptr;
+    if (!is_variable_len)	// only allocate for fixed-len string
+    {
+	strg_C = new char [attr_size+1];
+	ptr = strg_C;
+    }
+    else
+    {
+	// no allocation for variable-len string; C library will
+	ptr = &strg_C;
+    }
 
-   // Get 'string' from the C char* and release resource
-   strg_C[attr_size] = '\0';
-   strg = strg_C;
-   delete []strg_C;
+    // Call C API to get the attribute data, a string of chars
+    herr_t ret_value = H5Aread(id, mem_type.getId(), ptr);
+
+    // Get string from the C char* and release resource allocated locally
+    if (!is_variable_len)
+    {
+	strg_C[attr_size] = '\0';
+	strg = strg_C;
+	delete []strg_C;
+    }
+    // Get string from the C char* and release resource allocated by C API
+    else
+    {
+	strg = strg_C;
+	HDfree(strg_C);
+    }
+    ptr = NULL;
 }
 
 //--------------------------------------------------------------------------
@@ -217,11 +235,11 @@ hid_t Attribute::p_get_type() const
 }
 
 //--------------------------------------------------------------------------
-// Function:    Attribute::getFileName
-///\brief       Gets the name of the file, in which this attribute belongs.
-///\return      File name
-///\exception   H5::IdComponentException
-// Programmer   Binh-Minh Ribler - Jul, 2004
+// Function:	Attribute::getFileName
+///\brief	Gets the name of the file, in which this attribute belongs.
+///\return	File name
+///\exception	H5::IdComponentException
+// Programmer	Binh-Minh Ribler - Jul, 2004
 //--------------------------------------------------------------------------
 H5std_string Attribute::getFileName() const
 {
@@ -381,9 +399,6 @@ void Attribute::setId(const hid_t new_id)
     }
    // reset object's id to the given id
    id = new_id;
-
-   // increment the reference counter of the new id
-   incRefCount();
 }
 
 //--------------------------------------------------------------------------
@@ -402,8 +417,10 @@ void Attribute::close()
 	{
 	    throw AttributeIException("Attribute::close", "H5Aclose failed");
 	}
-	// reset the id because the attribute that it represents is now closed
-	id = 0;
+	// reset the id when the attribute that it represents is no longer
+	// referenced
+	if (getCounter() == 0)
+	    id = 0;
     }
 }
 
@@ -419,18 +436,11 @@ void Attribute::close()
 //--------------------------------------------------------------------------
 Attribute::~Attribute()
 {
-    int counter = getCounter(id);
-    if (counter > 1)
-    {
-	decRefCount(id);
+    try {
+	close();
     }
-    else if (counter == 1)
-    {
-	try {
-	    close();
-	} catch (Exception close_error) {
-	    cerr << "Attribute::~Attribute - " << close_error.getDetailMsg() << endl;
-	}
+    catch (Exception close_error) {
+	cerr << "Attribute::~Attribute - " << close_error.getDetailMsg() << endl;
     }
 }
 
diff --git a/c++/src/H5CommonFG.cpp b/c++/src/H5CommonFG.cpp
index 364f639..0217df2 100644
--- a/c++/src/H5CommonFG.cpp
+++ b/c++/src/H5CommonFG.cpp
@@ -15,11 +15,6 @@
 
 #include <string>
 
-// remove when done
-#include <iostream>
-    using std::cerr;
-    using std::endl;
-
 #include "H5Include.h"
 #include "H5Exception.h"
 #include "H5IdComponent.h"
diff --git a/c++/src/H5DataSet.cpp b/c++/src/H5DataSet.cpp
index be80949..5ee84b1 100644
--- a/c++/src/H5DataSet.cpp
+++ b/c++/src/H5DataSet.cpp
@@ -69,11 +69,10 @@ DataSet::DataSet(const hid_t existing_id) : AbstractDs(), H5Object()
 ///\param	original - IN: DataSet instance to copy
 // Programmer	Binh-Minh Ribler - 2000
 //--------------------------------------------------------------------------
-DataSet::DataSet(const DataSet& original) : AbstractDs(original), H5Object()
+DataSet::DataSet(const DataSet& original) : AbstractDs(original), H5Object(original)
 {
     id = original.getId();
     incRefCount(); // increment number of references to this id
-
 }
 
 //--------------------------------------------------------------------------
@@ -559,7 +558,7 @@ void DataSet::setId(const hid_t new_id)
    id = new_id;
 
    // increment the reference counter of the new id
-   incRefCount();
+   //incRefCount();
 }
 
 //--------------------------------------------------------------------------
@@ -578,8 +577,10 @@ void DataSet::close()
 	{
 	    throw DataSetIException("DataSet::close", "H5Dclose failed");
 	}
-	// reset the id because the dataset that it represents is now closed
-	id = 0;
+	// reset the id when the dataset that it represents is no longer
+	// referenced
+	if (getCounter() == 0)
+	    id = 0;
     }
 }
 
diff --git a/c++/src/H5DataSpace.cpp b/c++/src/H5DataSpace.cpp
index adeb2db..baad137 100644
--- a/c++/src/H5DataSpace.cpp
+++ b/c++/src/H5DataSpace.cpp
@@ -593,9 +593,6 @@ void DataSpace::setId(const hid_t new_id)
     }
    // reset object's id to the given id
    id = new_id;
-
-   // increment the reference counter of the new id
-   incRefCount();
 }
 
 //--------------------------------------------------------------------------
@@ -615,8 +612,10 @@ void DataSpace::close()
 	{
 	    throw DataSpaceIException("DataSpace::close", "H5Sclose failed");
 	}
-	// reset the id because the dataspace that it represents is now closed
-	id = 0;
+	// reset the id when the dataspace that it represents is no longer
+	// referenced
+	if (getCounter() == 0)
+	    id = 0;
     }
 }
 
@@ -632,16 +631,10 @@ void DataSpace::close()
 //--------------------------------------------------------------------------
 DataSpace::~DataSpace()
 {
-    int counter = getCounter(id);
-    if (counter > 1)
-	decRefCount(id);
-    else if (counter == 1)
-    {
-	try {
-	    close();
-	} catch (Exception close_error) {
-	    cerr << "DataSpace::~DataSpace - " << close_error.getDetailMsg() << endl;
-	}
+    try {
+	close();
+    } catch (Exception close_error) {
+	cerr << "DataSpace::~DataSpace - " << close_error.getDetailMsg() << endl;
     }
 }
 
diff --git a/c++/src/H5DataType.cpp b/c++/src/H5DataType.cpp
index 592d800..9575823 100644
--- a/c++/src/H5DataType.cpp
+++ b/c++/src/H5DataType.cpp
@@ -717,9 +717,6 @@ void DataType::setId(const hid_t new_id)
     }
    // reset object's id to the given id
    id = new_id;
-
-   // increment the reference counter of the new id
-   incRefCount();
 }
 
 //--------------------------------------------------------------------------
@@ -738,8 +735,10 @@ void DataType::close()
 	{
 	    throw DataTypeIException(inMemFunc("close"), "H5Tclose failed");
 	}
-	// reset the id because the datatype that it represents is now closed
-	id = 0;
+	// reset the id when the datatype that it represents is no longer
+	// referenced
+	if (getCounter() == 0)
+	    id = 0;
     }
 }
 
@@ -755,19 +754,11 @@ void DataType::close()
 //--------------------------------------------------------------------------
 DataType::~DataType()
 {
-    int counter = getCounter(id);
-    if (counter > 1)
-    {
-	decRefCount(id);
-    }
-    else if (counter == 1)
-    {
 	try {
 	    close();
 	} catch (Exception close_error) {
 	    cerr << inMemFunc("~DataType - ") << close_error.getDetailMsg() << endl;
 	}
-    }
 }
 #ifndef H5_NO_NAMESPACE
 } // end namespace
diff --git a/c++/src/H5File.cpp b/c++/src/H5File.cpp
index 9f8e45f..7cd1936 100644
--- a/c++/src/H5File.cpp
+++ b/c++/src/H5File.cpp
@@ -754,9 +754,6 @@ void H5File::setId(const hid_t new_id)
     }
    // reset object's id to the given id
    id = new_id;
-
-   // increment the reference counter of the new id
-   incRefCount();
 }
 
 //--------------------------------------------------------------------------
@@ -775,8 +772,10 @@ void H5File::close()
 	{
 	    throw FileIException("H5File::close", "H5Fclose failed");
 	}
-	// reset the id because the file that it represents is now closed
-	id = 0;
+	// reset the id when the file that it represents is no longer
+	// referenced
+	if (getCounter() == 0)
+	    id = 0;
     }
 }
 
@@ -813,18 +812,10 @@ void H5File::throwException(const H5std_string& func_name, const H5std_string& m
 //--------------------------------------------------------------------------
 H5File::~H5File()
 {
-    int counter = getCounter(id);
-    if (counter > 1)
-    {
-	decRefCount(id);
-    }
-    else if (counter == 1)
-    {
-	try {
-	    close();
-	} catch (Exception close_error) {
-	    cerr << "H5File::~H5File - " << close_error.getDetailMsg() << endl;
-	}
+    try {
+	close();
+    } catch (Exception close_error) {
+	cerr << "H5File::~H5File - " << close_error.getDetailMsg() << endl;
     }
 }
 
diff --git a/c++/src/H5Group.cpp b/c++/src/H5Group.cpp
index 7ab3b61..6014466 100644
--- a/c++/src/H5Group.cpp
+++ b/c++/src/H5Group.cpp
@@ -192,9 +192,6 @@ void Group::setId(const hid_t new_id)
     }
    // reset object's id to the given id
    id = new_id;
-
-   // increment the reference counter of the new id
-   incRefCount();
 }
 
 //--------------------------------------------------------------------------
@@ -206,8 +203,6 @@ void Group::setId(const hid_t new_id)
 //--------------------------------------------------------------------------
 void Group::close()
 {
- /* cerr << "Group::close/p_valid_id" << endl;
- */ 
     if (p_valid_id(id))
     {
 	herr_t ret_value = H5Gclose( id );
@@ -215,8 +210,10 @@ void Group::close()
 	{
 	    throw GroupIException("Group::close", "H5Gclose failed");
 	}
-	// reset the id because the group that it represents is now closed
-	id = 0;
+	// reset the id when the group that it represents is no longer
+	// referenced
+	if (getCounter() == 0)
+	    id = 0;
     }
 }
 
diff --git a/c++/src/H5IdComponent.cpp b/c++/src/H5IdComponent.cpp
index fa2f3db..b22d869 100644
--- a/c++/src/H5IdComponent.cpp
+++ b/c++/src/H5IdComponent.cpp
@@ -177,59 +177,13 @@ IdComponent& IdComponent::operator=( const IdComponent& rhs )
 
 	// copy the data members from the rhs object
 	setId(rhs.getId());
-	 /* id = rhs.id;
- */ 
+	incRefCount(getId()); // a = b, so there are two objects with the same
+			      // hdf5 id
     }
     return *this;
 }
 
 //--------------------------------------------------------------------------
-// Function:	IdComponent::setId
-///\brief	Sets the identifier of this object to a new value.
-///
-///\exception	H5::IdComponentException when the attempt to close the HDF5
-///		object fails
-// Description:
-// 		The underlaying reference counting in the C library ensures
-// 		that the current valid id of this object is properly closed.
-// 		Then the object's id is reset to the new id.
-// Programmer	Binh-Minh Ribler - 2000
-//--------------------------------------------------------------------------
-#if 0
-void IdComponent::setId(const hid_t new_id)
-{
-    // handling references to this old id
-    try {
-	close();
-    }
-    catch (Exception close_error) {
-	throw IdComponentException(inMemFunc("copy"), close_error.getDetailMsg());
-    }
-
-   // reset object's id to the given id
-    /* id = new_id;
- */ 
-   setId(new_id);
-
-   // increment the reference counter of the new id
-   incRefCount();
-}
-#endif
-
-//--------------------------------------------------------------------------
-// Function:	IdComponent::getId
-///\brief	Returns the id of this object
-///\return	HDF5 id
-// Programmer	Binh-Minh Ribler - 2000
-//--------------------------------------------------------------------------
-/*
-hid_t IdComponent::getId () const
-{
-   return(id);
-}
-*/
-
-//--------------------------------------------------------------------------
 // Function:	IdComponent destructor
 ///\brief	Noop destructor.
 // Programmer	Binh-Minh Ribler - 2000
@@ -273,10 +227,7 @@ H5std_string IdComponent::inMemFunc(const char* func_name) const
 ///\brief	Default constructor.
 // Programmer	Binh-Minh Ribler - 2000
 //--------------------------------------------------------------------------
-IdComponent::IdComponent() {
-    /* setId(-1);
- */ 
-}
+IdComponent::IdComponent() {}
 
 //--------------------------------------------------------------------------
 // Function:	IdComponent::p_get_file_name (protected)
@@ -335,31 +286,6 @@ hid_t IdComponent::p_dereference(void* ref)
    return(temp_id);
 }
 
-//--------------------------------------------------------------------------
-// Function:	IdComponent::p_reference (protected)
-// Purpose	Creates a reference to an HDF5 object or a dataset region.
-// Parameters
-//		name - IN: Name of the object to be referenced
-//		dataspace - IN: Dataspace with selection
-//		ref_type - IN: Type of reference; default to \c H5R_DATASET_REGION
-// Return	A reference
-// Exception	H5::IdComponentException
-// Notes	This function is incorrect, and will be removed in the near
-//		future after notifying users of the new APIs ::reference's.
-//		BMR - Oct 8, 2006
-// Programmer	Binh-Minh Ribler - May, 2004
-//--------------------------------------------------------------------------
- /* void* IdComponent::p_reference(const char* name, hid_t space_id, H5R_type_t ref_type) const
-{
-   hobj_ref_t ref;
-   herr_t ret_value = H5Rcreate(&ref, getId(), name, ref_type, space_id);
-   if (ret_value < 0)
-   {
-      throw IdComponentException("", "H5Rcreate failed");
-   }
-   return (reinterpret_cast<void*>(ref));
-}
- */ 
 //
 // Local functions used in this class
 //
diff --git a/c++/src/H5PropList.cpp b/c++/src/H5PropList.cpp
index 64c1743..085dfd1 100644
--- a/c++/src/H5PropList.cpp
+++ b/c++/src/H5PropList.cpp
@@ -53,7 +53,7 @@ PropList::PropList() : IdComponent(), id(0) {}
 ///\param	original - IN: The original property list to copy
 // Programmer	Binh-Minh Ribler - 2000
 //--------------------------------------------------------------------------
-PropList::PropList(const PropList& original) : IdComponent(original) 
+PropList::PropList(const PropList& original) : IdComponent(original)
 {
     id = original.getId();
     incRefCount(); // increment number of references to this id
@@ -245,9 +245,6 @@ void PropList::setId(const hid_t new_id)
     }
    // reset object's id to the given id
    id = new_id;
-
-   // increment the reference counter of the new id
-   incRefCount();
 }
 
 //--------------------------------------------------------------------------
@@ -266,8 +263,10 @@ void PropList::close()
 	{
 	    throw PropListIException(inMemFunc("close"), "H5Pclose failed");
 	}
-	// reset the id because the property list that it represents is now closed
-	id = 0;
+	// reset the id when the property list that it represents is no longer
+	// referenced
+	if (getCounter() == 0)
+	    id = 0;
     }
 }