nsComponentManager.cpp: do not use 'else' after 'return'
Categories
(Core :: XPCOM, task)
Tracking
()
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.
Comment 1•4 years ago
|
||
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•4 years ago
|
||
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?
Assignee | ||
Comment 3•4 years ago
|
||
Comment 4•4 years ago
|
||
(In reply to Sylvestre Ledru [:Sylvestre] from comment #3)
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.
Assignee | ||
Comment 5•4 years ago
|
||
I am using trunk (clang 12 from apt.llvm.org). So, maybe a different upstream bug.
Comment 6•4 years ago
|
||
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•4 years ago
•
|
||
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 | ||
Comment 8•4 years ago
|
||
Updated•4 years ago
|
Comment 9•4 years ago
|
||
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•4 years ago
|
||
Comment 11•4 years ago
|
||
bugherder |
Description
•