Remove/update confusing comments. Separate acquire and assign operati…

…ons for pointer functions.
gnustep · Jul 17, 2024 · c435c6d · c435c6d
1 parent f6b8c83
commit c435c6d
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 47 deletions.
diff --git a/Source/NSConcreteHashTable.m b/Source/NSConcreteHashTable.m
@@ -90,7 +90,8 @@ @interface	NSConcreteHashTable : NSHashTable
   : IS_WEAK(M) ? nil : pointerFunctionsRelinquish(&M->cb.pf, &X.ptr))
 #define GSI_MAP_RETAIN_KEY(M, X)\
  (M->legacy ? M->cb.old.retain(M, X.ptr) \
-  : IS_WEAK(M) ? nil : pointerFunctionsAcquire(&M->cb.pf, &X.ptr, X.ptr))
+  : IS_WEAK(M) ? nil : pointerFunctionsAssign(\
+    &M->cb.pf, &X.ptr, pointerFunctionsAcquire(&M->cb.pf, X.ptr)))
 #define GSI_MAP_ZEROED(M)\
  (M->legacy ? 0 \
  : (IS_WEAK(M) ? YES : NO))

diff --git a/Source/NSConcreteMapTable.m b/Source/NSConcreteMapTable.m
@@ -98,49 +98,42 @@ @interface	NSConcreteMapTable : NSMapTable
   : IS_WEAK_KEY(M) ? nil : pointerFunctionsRelinquish(&M->cb.pf.k, &X.ptr))
 #define GSI_MAP_RETAIN_KEY(M, X)\
  (M->legacy ? M->cb.old.k.retain(M, X.ptr) \
-  : IS_WEAK_KEY(M) ? nil : pointerFunctionsAcquire(&M->cb.pf.k, &X.ptr, X.ptr))
+  : IS_WEAK_KEY(M) ? nil : pointerFunctionsAssign(&M->cb.pf.k, &X.ptr,\
+    pointerFunctionsAcquire(&M->cb.pf.k, X.ptr)))
 #define GSI_MAP_RELEASE_VAL(M, X)\
  (M->legacy ? M->cb.old.v.release(M, X.ptr) \
   : IS_WEAK_VALUE(M) ? nil : pointerFunctionsRelinquish(&M->cb.pf.v, &X.ptr))
 #define GSI_MAP_RETAIN_VAL(M, X)\
  (M->legacy ? M->cb.old.v.retain(M, X.ptr) \
-  : IS_WEAK_VALUE(M) ? nil : pointerFunctionsAcquire(&M->cb.pf.v, &X.ptr, X.ptr))
+  : IS_WEAK_VALUE(M) ? nil : pointerFunctionsAssign(&M->cb.pf.v, &X.ptr,\
+    pointerFunctionsAcquire(&M->cb.pf.v, X.ptr)))
 
-/* 2013-05-25 Here are the macros originally added for GC/ARC ...
- * but they caused map table entries to be doubly retained :-(
- * The question is, are the new versions I hacked in below to
- * fix this correct?
- 
-#define GSI_MAP_WRITE_KEY(M, addr, x) \
-	if (M->legacy) \
-	  *(addr) = x;\
-	else\
-	  pointerFunctionsAssign(&M->cb.pf.k, (void**)addr, (x).obj);
-#define GSI_MAP_WRITE_VAL(M, addr, x) \
-	if (M->legacy) \
-	  *(addr) = x;\
-	else\
-	  pointerFunctionsAssign(&M->cb.pf.v, (void**)addr, (x).obj);
-*/
+/* The GSI_MAP_WRITE_KEY() and GSI_MAP_WRITE_VAL() macros are expectd to
+ * write without retain (GSI_MAP RETAIN macros are executed separately)
+ * so they can't use pointerFunctionsAssign() unless working with weak
+ * references.
+ */
 #define GSI_MAP_WRITE_KEY(M, addr, x) \
-	if (M->legacy) \
-          *(addr) = x;\
-	else\
-	  (IS_WEAK_KEY(M) ? pointerFunctionsAssign(&M->cb.pf.k, (void**)addr, (x).obj) : (*(id*)(addr) = (x).obj));
+  if (M->legacy) \
+    *(addr) = x;\
+  else\
+    (IS_WEAK_KEY(M) ? pointerFunctionsAssign(&M->cb.pf.k, (void**)addr,\
+      (x).obj) : (*(id*)(addr) = (x).obj));
 #define GSI_MAP_WRITE_VAL(M, addr, x) \
-	if (M->legacy) \
-          *(addr) = x;\
-	else\
-	  (IS_WEAK_VALUE(M) ? pointerFunctionsAssign(&M->cb.pf.v, (void**)addr, (x).obj) : (*(id*)(addr) = (x).obj));
+  if (M->legacy) \
+    *(addr) = x;\
+  else\
+    (IS_WEAK_VALUE(M) ? pointerFunctionsAssign(&M->cb.pf.v, (void**)addr,\
+      (x).obj) : (*(id*)(addr) = (x).obj));
 #define GSI_MAP_READ_KEY(M,addr) \
-	(M->legacy ? *(addr)\
-	  : (__typeof__(*addr))pointerFunctionsRead(&M->cb.pf.k, (void**)addr))
+  (M->legacy ? *(addr)\
+    : (__typeof__(*addr))pointerFunctionsRead(&M->cb.pf.k, (void**)addr))
 #define GSI_MAP_READ_VALUE(M,addr) \
-	(M->legacy ? *(addr)\
-	  : (__typeof__(*addr))pointerFunctionsRead(&M->cb.pf.v, (void**)addr))
+  (M->legacy ? *(addr)\
+    : (__typeof__(*addr))pointerFunctionsRead(&M->cb.pf.v, (void**)addr))
 #define GSI_MAP_ZEROED(M)\
-        (M->legacy ? 0\
-	  : (IS_WEAK_KEY(M) || IS_WEAK_VALUE(M)) ? YES : NO)
+  (M->legacy ? 0\
+    : (IS_WEAK_KEY(M) || IS_WEAK_VALUE(M)) ? YES : NO)
 
 #define	GSI_MAP_ENUMERATOR	NSMapEnumerator
 

diff --git a/Source/NSConcretePointerFunctions.h b/Source/NSConcretePointerFunctions.h
@@ -139,23 +139,17 @@ static inline void pointerFunctionsAssign(PFInfo *PF, void **addr, void *value)
     }
 }
 
-/* Acquire the pointer value to store for the specified item.
- */
 static inline void *
-pointerFunctionsAcquire(PFInfo *PF, void **dst, void *src)
+pointerFunctionsAcquire(PFInfo *PF, void *src)
 {
   if (PF->acquireFunction != 0)
-    src = (*PF->acquireFunction)(src, PF->sizeFunction,
-    PF->options & NSPointerFunctionsCopyIn ? YES : NO);
-  // FIXME: This shouldn't be here.  Acquire and assign are separate
-  // operations.  Acquire is for copy-in operations (i.e. retain / copy),
-  // assign is for move operations of already-owned pointers.  Combining them
-  // like this is Just Plain Wrong™
-  pointerFunctionsAssign(PF, dst, src);
+    {
+      src = (*PF->acquireFunction)(src, PF->sizeFunction,
+	PF->options & NSPointerFunctionsCopyIn ? YES : NO);
+    }
   return src;
 }
 
-
 /**
  * Moves a pointer from location to another.
  */

diff --git a/Source/NSPointerArray.m b/Source/NSPointerArray.m
@@ -308,8 +308,9 @@ - (id) copyWithZone: (NSZone*)zone
   for (i = 0; i < _count; i++)
     {
       NSLog(@"Copying %d, %p", i, _contents[i]);
-      pointerFunctionsAcquire(&_pf, &c->_contents[i],
-              pointerFunctionsRead(&_pf, &_contents[i]));
+      pointerFunctionsAssign(&_pf, &c->_contents[i],
+        pointerFunctionsAcquire(&_pf,
+	  pointerFunctionsRead(&_pf, &_contents[i])));
     }
   return c;
 }
@@ -442,7 +443,8 @@ - (void) insertPointer: (void*)pointer atIndex: (NSUInteger)index
       pointerFunctionsMove(&_pf, _contents+i, _contents + i-1);
       i--;
     }
-  pointerFunctionsAcquire(&_pf, &_contents[index], pointer);
+  pointerFunctionsAssign(&_pf, &_contents[index],
+    pointerFunctionsAcquire(&_pf, pointer));
 }
 
 - (BOOL) isEqual: (id)other