824840 - Easy to forget the argument to MOZ_ASSERT_UNREACHABLE()

Status: RESOLVED WORKSFORME

(Core :: MFBT, defect)

Description

[Nicholas Nethercote [:njn]]

We have 16 places in the code base where MOZ_NOT_REACHED() is missing an argument.  clang 3.2 warns about these, and if you build with --enable-warnings-as-errors it causes the build to fail.

The simple fix is to add the argument to the 16 occurrences, but I wonder if we should make the zero-argument case legitimate instead?  This is an easy mistake to make.

Comment 1

[Daniel Holbert [:dholbert]]

(Technically, MOZ_NOT_REACHED_MARKER() is already a no-argument version of MOZ_NOT_REACHED(), but it doesn't look like it's intended for use outside of other macros' implementations in Assertions.h.)

Comment 2

[Justin Lebar (not reading bugmail)]

> I wonder if we should make the zero-argument case legitimate instead?

+1  MOZ_NOT_REACHED("no") is not useful.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Would this require a multi-step macro declaration like MOZ_ASSERT (http://dxr.mozilla.org/mozilla-central/mfbt/Assertions.h.html#l238)?

Or is it as simple as changing MOZ_NOT_REACHED(reason) to MOZ_NOT_REACHED(...) and using __VA_ARGS__?

I haven't used Visual C++, but this is stopping me from switching to Clang on linux so I'm willing to fix it.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

This was renamed to MOZ_ASSUME_UNREACHABLE() and I can see only one use without a argument, which is in some dummy initializer [1].

[1]: http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#1147

OK to close this as INVALID?

Comment 5

[Nicholas Nethercote [:njn]]

Sounds like the problem still exists, even if the macro's name changed.

Comment 6

[Chris Peterson [:cpeterson]]

See related bug 990764 for discussion of removing or replacing MOZ_ASSUME_UNREACHABLE.

Comment 7

[Chris Peterson [:cpeterson]]

Bug 990764 removed MOZ_ASSUME_UNREACHABLE, replacing calls with MOZ_ASSERT_UNREACHABLE and MOZ_CRASH.

Comment 8

[Chris Peterson [:cpeterson]]

We build with --enable-warnings-as-errors by default, so any missing macro arguments would break the build today.