Open Bug 1765916 Opened 3 years ago Updated 3 years ago

Neither `Result::isOk` nor `Result::isErr` has `MOZ_(UN)LIKELY`, but `NS_SUCCEEDED` and `NS_FAILED` have

Categories

(Core :: MFBT, enhancement)

Product:
Core
Component:
MFBT
Type:
enhancement

Tracking

()

People

(Reporter: masayuki, Unassigned)

Details

NS_SUCCEEDED and NS_FAILED have MOZ_LIKELY and MOZ_UNLIKELY. However, neither Result::isOk nor Result::isErr does not have. This inconsistency causes mixed style code like:

Result<Foo, nsresult> result = Bar();
if (MOZ_UNLIKELY(result.isErr())) {
  return result.unwrapErr();
}
nsresult rv = Baz();
if (NS_FAILED(rv)) {
  return rv;
}

So they should have them.

Result::isOk and Result:isErr are member functions. Does adding MOZ_LIKELY or MOZ_UNLIKELY annotations inside those functions' (inline) definitions allow the compiler to optimize the caller code? IIUC, MOZ_LIKELY or MOZ_UNLIKELY are a property of a conditional expression, not a property of a value that could be tracked across function calls and returns.

(In reply to Chris Peterson [:cpeterson] from comment #1)

Result::isOk and Result:isErr are member functions. Does adding MOZ_LIKELY or MOZ_UNLIKELY annotations inside those functions' (inline) definitions allow the compiler to optimize the caller code? IIUC, MOZ_LIKELY or MOZ_UNLIKELY are a property of a conditional expression, not a property of a value that could be tracked across function calls and returns.

Ah, that's a good point. I guess that the compilers do because of inline-able methods, but not guaranteed of course. My point is, the difference causes one is required but the other is not. Then, the style of callers-side becomes different. (Personally, I don't think that NS_FAILED should have it because failure may be an expected case if the caller has fallback path.)

I found a post. It was posted 7 years ago, but clang does not take the hint in inline methods at least in 3.7.

You need to log in before you can comment on or make changes to this bug.