172131 - FMM, Comments which suggest the wrong (De)Allocator, and Style

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[timeless]

nsCRT.h's comment suggests the wrong (De)Allocator.

nsComponentManager.cpp actually uses the wrong DeAllocator. The rest of this is
whitespace, unreachable/silly code, and minor cleanup.

Comment 1

[timeless]

Attachment: for review only

Comment 2

[timeless]

Attachment: the complete patch

Comment 3

[timeless]

Attachment: new patch -u25

Comment 4

[timeless]

Attachment: same changes -uw

Comment 5

[David Bradley]

Should you create an inline IsDoneImpl function so we don't incure nested
virtual calls? It would make it more efficient, but less flexable. Someone
overriding IsDone wouldn't necessarily get the behavior they expect. But I don't
think that's a big issue for how this class would be used.

Comment 6

[timeless]

Attachment: leaving IsDone alone for now

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 102082 [details] [diff] [review]
leaving IsDone alone for now

Why?

     PR_Free(cidString);
-    PR_FREEIF(contractID);
-    PR_FREEIF(className);
+    if (contractID)
+	 PR_Free(contractID);
+    if (className)
+	 PR_Free(className);
+

Revert that please and r=dougt

Comment 8

[timeless]

http://lxr.mozilla.org/nspr/source/nsprpub/pr/include/prmem.h#152

140 #define PR_DELETE(_ptr) { PR_Free(_ptr); (_ptr) = NULL; }
152 #define PR_FREEIF(_ptr) if (_ptr) PR_DELETE(_ptr)

the result is that PR_FREEIF wastefully assigns null to local variables which
are about to leave scope.

Comment 9

[Darin Fisher]

Comment on attachment 102082 [details] [diff] [review]
leaving IsDone alone for now

sr=darin

Comment 10

[Doug Turner (:dougt)]

i wouldn't be surprised if the set was optimized out.  i retract my nit.

Comment 11

[timeless]

fixed
filed bug 173260 for IsDone