Closed Bug 398952 Opened 13 years ago Closed 13 years ago
Rewrite the ns
COMPtr tests to be runnable from |make check|
The output of the current tests is hard to read, difficult to use to verify correctness, and can't be run automatically, so if someone broke it I'm quite sure they wouldn't actually find out through a testcase (as opposed to an obscure bug in some Mozilla code that happened to die because of it). The test code itself is also disorganized and not easily readable. I basically converted the entire testcase to be automatically runnable and readable. I removed a test that used nsGetterAddRefs directly (per comments in nsCOMPtr those should have been getter_AddRefs), and I added a couple tests for |swap|, and I might have removed one or two other things (hard to remember), but it's basically the same thing, albeit with much more controlled output and more deliberate testing. This all gets a bit grungy, so I suggest you consider the option of rubber-stamping this. (If you don't, I strongly suggest you ignore the diff and just read the new file instead, or read the right-hand column only in the patch viewer. You *do not* want to read the old version.) It was probably only barely worth the time spent doing this, and I doubt a meticulous double-check is worth the time. There's overlap in the area of code touched between what's here and what's in bug 397227, but I think this patch plus adding TestCOMPtr to CPP_UNIT_TESTS shouldn't cause conflicts with anything.
Attachment #283963 - Flags: review?(benjamin)
This will conflict terribly with moz2 work, so I'm going to mark this WONTFIX.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
Um, terribly? It's a single file, and I see no reason the standard tools wouldn't work for fixing up the file -- there's nothing non-standard about its uses of nsCOMPtr. If they don't, I can't imagine it would be difficult to manually make the changes.
Comment on attachment 283963 [details] [diff] [review] Patch COMPtr in mozilla2 shares only superficial semantics compared to what we have now, and the current test is worthless. Rather than spending time merging it, I'd prefer to ignore it for the time being.
Attachment #283963 - Flags: review?(benjamin) → review-
You need to log in before you can comment on or make changes to this bug.