1301574 - Introduce mozilla::XXXUnused

Status: RESOLVED WONTFIX

(Core :: MFBT, defect)

Description

[Nicholas Nethercote [inactive]]

I think it will be useful to have a variant of |Unused| that indicates "we
deliberately ignore this value but we probably shouldn't".

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Introduce mozilla::XXXUnused

Comment 2

[Mike Hommey [:glandium]]

Why not something clearer like "ShouldBeUsed"? (or XXXShouldBeUsed)

Comment 3

[Nicholas Nethercote [inactive]]

I expected bikeshedding about the name, that's fine. But not that at it's possible that not using the value is reasonable, so ProbablyShouldBeUsed would be more accurate (though unwieldy).

Comment 4

[Jeff Walden [:Waldo]]

Explain why it isn't feasible to just fix the stupid code?  I get the sigil-thing for switching to a new API, sometimes.  But checking return values is just not on the same plane at all.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Explain why it isn't feasible to just fix the stupid code?

See the patch for three particular examples. They all came from bug 1297300 for an example of how much effort it is to do such fixes. There were a couple of hundred missing checks for GetSpec().

Sometimes papering over the current flaws in order to prevent new ones is a good idea, but if you do that, it's nice to have a clear indication where you did the papering.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8789626 [details] [diff] [review]
Introduce mozilla::XXXUnused

Review of attachment 8789626 [details] [diff] [review]:
-----------------------------------------------------------------

Is this really any more helpful than the XXX comments already at all the callsites that you're modifying?  "Yes," I hear you say, "searching for XXX turns up any number of things, whereas searching for XXXUnused limits the search to these particular sites."  OK, how about simply changing the comments to read XXXUnused or XXXMustUse?  I get wanting to identify all the places where we should go back and re-examine papering over problems, I'm just not convinced that doing it in code is necessarily better than comments.

Comment 7

[Nicholas Nethercote [inactive]]

> OK, how about simply changing the comments to read XXXUnused or XXXMustUse?

It's less reliable -- e.g. the compiler won't complain if you misspell them. Also, there's the possibility of changing the implementation of XXXUnused to actually do something, such as print a warning when the unused value is a failure, which might help identify which ones should be fixed first. (One probably wouldn't land a patch that does that, but it might be a useful thing to in a local build.)

But if you're really opposed just WONTFIX this and I will make do with comments.