Example (from https://phabricator.services.mozilla.com/D84371#inline-484201)

if (Result<nsINode*, nsresult> lastNode = DetermineLastNode();
      NS_WARN_IF(lastNode.isErr())) {
  return lastNode.unwrapErr();
} else {
  mIterator.mLast = lastNode.unwrap();
}

See https://en.cppreference.com/w/cpp/language/if.

The advantage of declaring lastNode like that is that its scope is limited to the if- and else-block. So this shouldn't cause a warning.

The severity field is not set for this bug.
:andi, could you have a look please?

For more information, please visit auto_nag documentation.

While there may be cases of this not involving mozilla::Result, for this particular case, mod. the NS_WARN_IF, this could be written much more concise as MOZ_TRY_VAR(mIterator.mLast, DetermineLastNode()).

We have come across this deficiency of MOZ_TRY/MOZ_TRY_VAR when revamping the error handling in Quota Manager and its clients. In dom/quota/QuotaCommon.h, we have extended the MOZ_TRY macro towards supporting issuing warnings (and several other improvements). We plan to generalize these into mfbt/Result.h at some point. One challenge for that is dealing with component-specific error reporting (for QM and its clients, we want to add telemetry for that, e.g., but that may not be appropriate across the codebase).

I'm a newbie and don't know the logic of the Reviewbot, but just wanted to point out that this comes from clang-tidy, see the example below. I tend to agree that this is an incorrectly flagged code, but that would be a bug report for clang.

test.cpp

#include <iostream>

bool iseven(int);
bool ispositive(double);

int main() {
  if (int x=42; iseven(x)) {
    return x;
  }
  else {
    std::cout << x << " is odd" << std::endl;
  }

  double x = 3.14;

  return ispositive(x);
}

clang-tidy

$ clang-tidy -checks=-*,readability-else-* test.cpp 
/tmp/test.cpp:10:3: warning: do not use 'else' after 'return' [readability-else-after-return]
  else {
  ^

Rafal, I agree with you and this is a great example (even if this a quite ugly example).

Could you please report a bug upstream?
https://bugs.llvm.org/enter_bug.cgi
(I can do it if you want).

Let's not hurry to open a bug since we have https://github.com/llvm/llvm-project/blame/dba33258ff6f2c14f32e60d270dd00f7629be856/clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp#L37.

it's an intended behavior, if we really want this we can have readability-else-after-return.WarnOnConditionVariables, value: false

Oh yeah, well spotted:
https://reviews.llvm.org/D82824
the option is available from clang-tidy 11. Maybe we should enable it

(In reply to Sylvestre Ledru [:Sylvestre] from comment #6)

Oh yeah, well spotted:
https://reviews.llvm.org/D82824
the option is available from clang-tidy 11. Maybe we should enable it

Yes, after we migrate to clang-11 we can enable this, even though I'm not a huge fan of this syntax sugar.

Release notes: https://releases.llvm.org/11.0.0/tools/clang/tools/extra/docs/ReleaseNotes.html#changes-in-existing-checks

Already have this in tree.