Closed Bug 1674633 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: XPCOM, task)

task

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox86 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=C++])

Attachments

(1 file)

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.

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?

Flags: needinfo?(sledru)

(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.

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

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.

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.

Assignee: nobody → sledru
Status: NEW → ASSIGNED

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.

Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9fa12cba9cd8 clang-tidy: disable WarnOnConditionVariables for readability-else-after-return r=static-analysis-reviewers,andi DONTBUILD
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: