IMO the MOZ_GUARD_OBJECT_etc macros are ugly to look at and clunky to use. I admit to deliberately not using them sometimes. If we could mark RAII objects with a single annotation on the class, and let static analysis do the rest, I think we'd have nicer code and broader coverage.

We should add a MOZ_RAII attribute that implies MOZ_STACK_CLASS, and also enforces that constructing an unnamed temporary of the type should be disallowed. Michael, are you interested?

(In reply to Ehsan Akhgari (don't ask for review please) from comment #1) > We should add a MOZ_RAII attribute that implies MOZ_STACK_CLASS, and also > enforces that constructing an unnamed temporary of the type should be > disallowed. > > Michael, are you interested? Sure. I'd probably add `MOZ_NON_TEMPORARY_CLASS` (which should be pretty trivial with the new allocation analysis framework) and then `#define MOZ_RAII MOZ_NON_TEMPORARY_CLASS MOZ_STACK_CLASS`.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab2a3e472439

Comment on attachment 8656755 [details] [diff] [review] Part 3: Mark every consumer of GUARD_OBJECT as MOZ_RAII Review of attachment 8656755 [details] [diff] [review]: ----------------------------------------------------------------- Why are you not removing the GuardObject stuff?

(In reply to Ehsan Akhgari (don't ask for review please) from comment #7) > Comment on attachment 8656755 [details] [diff] [review] > Part 3: Mark every consumer of GUARD_OBJECT as MOZ_RAII > > Review of attachment 8656755 [details] [diff] [review]: > ----------------------------------------------------------------- > > Why are you not removing the GuardObject stuff? Mostly because doing this was already somewhat tedious, and I wasn't sure if we would want to remove the GuardObject stuff, so I left it until later. If we don't want GuardObject anymore, I can purge it, but I think I'd want that to be a seperate patch.

Hmm, if we don't drop the GuardObject stuff then this would be less valuable... Nathan, what do you think?

My real motivation for filing was to get rid of the guard macros. Moving their functionality to static analysis is just a means to that end :)

(In reply to David Major [:dmajor] from comment #10) > My real motivation for filing was to get rid of the guard macros. Moving > their functionality to static analysis is just a means to that end :) If people want the guard macros gone, then we might as well just get rid of them completely. I'll make a part 4 patch which completely removes the guard macros.

(In reply to Ehsan Akhgari (don't ask for review please) from comment #9) > Hmm, if we don't drop the GuardObject stuff then this would be less > valuable... Nathan, what do you think? I like the idea of dropping the guard object stuff, partly because some other annotation on the class is easier on the eyes, and partly because the guard object checks can be reasonably expensive (i.e. show up in profiles) on debug builds. I am a little worried about dropping this support on non-static analysis platforms (i.e. Windows). I guess if we got clang-cl builds up and running on infra, we could just turn on static analysis there? I don't really know what the potential fallout would be like on Windows if people started making mistakes that the guards would have caught. (I realize the same objections apply to all our other static analyses, of course, but those have tended to add checks; the proposed guard object removal would be removing checks, which seems bad.) On balance, I am slightly against removing the guard object stuff for now. Thoughts?

That makes me sad. I can't really argue with your reasoning, though. :-/

I suppose we should open a bug up depending on both this and bug 752004 to actually completely remove the guard object stuff?

Bug 752004 is the wrong bug, but yeah, we should keep them both for now, it seems.

Comment on attachment 8656754 [details] [diff] [review] Part 2: Add MOZ_NON_TEMPORARY_CLASS and MOZ_RAII to mfbt LGTM.

Comment on attachment 8656755 [details] [diff] [review] Part 3: Mark every consumer of GUARD_OBJECT as MOZ_RAII Review of attachment 8656755 [details] [diff] [review]: ----------------------------------------------------------------- When you land this, please write to dev-platform and ask people to use this new attribute. Thanks!

Can you also file a followup to s/MOZ_STACK_CLASS/MOZ_RAII/ where appropriate? Thank you!

done - bug 1204239

https://hg.mozilla.org/integration/mozilla-inbound/rev/7af4eae9a68f https://hg.mozilla.org/integration/mozilla-inbound/rev/296fe95e64cb https://hg.mozilla.org/integration/mozilla-inbound/rev/31a26ed44484

https://hg.mozilla.org/mozilla-central/rev/7af4eae9a68f https://hg.mozilla.org/mozilla-central/rev/296fe95e64cb https://hg.mozilla.org/mozilla-central/rev/31a26ed44484