Closed Bug 1352816 Opened 5 years ago Closed 5 years ago

Description for NS_WARNING_ASSERTION should be optional

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: ayg, Unassigned)

References

Details

After bug 1299384, NS_WARN_IF(foo) no longer works if the return value is not used.  This means you need to do either

  Unused << NS_WARN_IF(foo);

or

  NS_WARNING_ASSERTION(foo, "foo");

Can we make the description optional so just

  NS_WARNING_ASSERTION(foo);

works?  Often the condition is self-documenting, e.g., NS_FAILED(res).

If this is wanted, I'll write the patch, unless variable-argument macros are harder than I assume.  :)
Flags: needinfo?(erahm)
I'd *really* prefer anything that outputs a warning to have a unique description. Take for example one file, foo.cpp:

void foo() {
  NS_WARNING_ASSERTION(NS_FAILED(rv));
}

void bar() {
  NS_WARNING_ASSERTION(NS_FAILED(rv));
}

The error that gets output will be identical except for line number. Now that may seem like enough to find the source of the warning, but as someone who bisects warning regressions I can tell you this makes things a lot harder. As code gets added and removed from the file the line number moves around. I get the NS_WARN_IF lets you do this and really I wish it didn't :(

If you'd like we can appeal to Nathan (or dev-platform) of course, I know I have rather extreme opinions on warnings!
Flags: needinfo?(erahm)
So the problem with line number is that if you're bisecting it makes it hard to tell which is which?  I get that, but on the other hand, that's always been the norm in the NS_ENSURE_* pattern.  As someone who doesn't bisect warning regressions but does write code that has warnings :), I think it would be a significant burden to come up with a unique warning for every NS_FAILED(rv) check.  I think people who write warnings are a lot more numerous than those who bisect them, so that should count for something.

Nathan, what do you think?
Flags: needinfo?(nfroyd)
My understanding from the above comments is that you want to write many:

  NS_WARNING_ASSERTION(NS_FAILED(rv));

checks and contend that writing a message for each one would be difficult.  I guess you want to write the assertion form, rather than the almost-equivalent:

  Unused << NS_WARN_IF(NS_FAILED(rv));

which already requires no messages, because you don't want the condition evaluated in release builds, per the documentation?  (Or because it's simply more convenient to type.)

If you think that what's happening is rare enough that warning is appropriate, then it doesn't seem like having to write a short message explaining what's going wrong is much of a burden.  If a warning message is for whatever reason not appropriate, using the alternate form above (which is less common than NS_WARNING_ASSERTION, though that might be because of difference in code age--I didn't check) also does not seem unduly burdensome.
Flags: needinfo?(nfroyd)
People also can be wildly wrong about how often warnings occur, so making them a little harder to write and/or easier to figure out where you're flooding logs with warnings also doesn't seem like a bad tradeoff.
Okay, fine by me.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
Generally my thoughts are:

#1 If you want to warn, but not abort the function we're all better served with a log message. This makes the verbosity opt-in and allows you to turn it on in release builds.

> MOZ_LOG(log, LogLevel::Warning, ("This thing unexpectedly failed but I don't really want to crash: %d", rv));

#2 If you don't want to use a log, want a cheap warning and don't handle the failure a MOZ_ASSERT is probably better. We'll get stacks for the erroneous condition, etc but let control continue in release builds.

> MOZ_ASSERT(NS_SUCCEEDED(rv));

If #1 is too onerous or doesn't provide enough info (line #, file, etc) and #2 isn't an option perhaps, for your code, we can add a wrapper of MOZ_LOG that includes those details like we do in layout [1]. This has the benefits of being opt-in, but also easy enough to use. It might make sense to generalize that in mozilla/Logging.h instead, if this is something you're interested in I'd be happy to work on it.

[1] http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/layout/base/LayoutLogging.h
You need to log in before you can comment on or make changes to this bug.