1674633 - nsComponentManager.cpp: do not use 'else' after 'return'

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Sylvestre Ledru [:Sylvestre]]

Filling as a good first bug to learn workflows.

do not use 'else' after 'return':
https://searchfox.org/mozilla-central/source/xpcom/components/nsComponentManager.cpp#1558-1563

As the change is trivial, it is just to learn how to contribute to Firefox.

Found by http://clang.llvm.org/extra/clang-tidy/checks/readability-else-after-return.html

Tutorial to contribute:
https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html

Please don't ask for the bug to be assigned. It will be automatically assigned to the first patch.

Comment 1

[jassheikh333]

I don't see any else in the mentioned lines #1558-1563:-
https://searchfox.org/mozilla-central/source/xpcom/components/nsComponentManager.cpp#1558-1563

Comment 2

[Andrew McCreight [:mccr8]]

There's an else after a return in nsComponentManagerImpl::RegisterFactory(), but we can't change it because the scope of the if covers the else.

There's another one in nsComponentManagerImpl::GetService(), but getting rid of the return would require making the if nested so it is a bit arguable whether it is an improvement.

Sylvestre, do you know if there are any current issues for this file?

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Sorry, it was
https://searchfox.org/mozilla-central/rev/c7cf087b6e1384608ca3989f042f12f7cabd0a5f/xpcom/components/nsComponentManager.cpp#1512

Comment 4

[Simon Giesecke [:sg] [he/him]]

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

Sorry, it was
https://searchfox.org/mozilla-central/rev/c7cf087b6e1384608ca3989f042f12f7cabd0a5f/xpcom/components/nsComponentManager.cpp#1512

As :mccr8 mentioned, if clang-tidy complains about that, it's a false positive, since there's an init-expression that's referenced in the else branch as well. The situation is similar to that reported in https://bugs.llvm.org/show_bug.cgi?id=44364, not sure in what clang-tidy version that got fixed and if it covers this case.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

I am using trunk (clang 12 from apt.llvm.org). So, maybe a different upstream bug.

Comment 6

[Simon Giesecke [:sg] [he/him]]

I can reproduce it with:

struct Foo {
  void Bar() {}

  operator bool() const { return true; }
};

Foo MakeFoo() { return {}; }

int main() {
    if (auto x = MakeFoo()) {
        return 42;
    } else {
        x.Bar();
        return 43;
    }
}

I filed https://bugs.llvm.org/show_bug.cgi?id=48543 for that.

Comment 7

[Andi [:andi]]

I don't want to bring the sad news here, but the implementation of this checker is very archaic and needs a major refactoring to include the current case and also initialize support.
What it does now is only match if and else:

  Finder->addMatcher(
      compoundStmt(
          forEach(ifStmt(hasThen(stmt(anyOf(returnStmt(),
                                            compoundStmt(has(returnStmt()))))),
                         hasElse(stmt().bind("else")))
                      .bind("if"))),
      this);

Also a major problem that I see here, it's the complexity level of the checker, it's very hard to understand what it tries to do deep inside it's internals and how it achieves this. looking at the test cases it should support init statemes, and this PR suggests this.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1674633 - clang-tidy: disable WarnOnConditionVariables for readability-else-after-return r?#static-analysis-reviewers

Comment 9

[Simon Giesecke [:sg] [he/him]]

Sylvestre, thanks for the configuration change. Admittedly, I didn't look carefully enough, but I still find it kind of surprising that the default configuration catches these cases, and suggests what can be understood as a kind of de-modernization, and the message doesn't even mention that.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9fa12cba9cd8
clang-tidy: disable WarnOnConditionVariables for readability-else-after-return r=static-analysis-reviewers,andi DONTBUILD

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/9fa12cba9cd8