Page MenuHomePhabricator

test: Add lint to prevent SIGNAL/SLOT connect style
ClosedPublic

Authored by nakihito on Oct 29 2019, 20:30.

Details

Summary

Added new linter that enforces Qt5 SIGNAL/SLOT connection style.

Partial backport of Core PR13529 (adjusted to work with arcanist)
https://github.com/bitcoin/bitcoin/pull/13529/commits/3567b247f43decb6fc102d5b0989d1746fce0441

Test Plan
arc lint --everything

No Qt5 code violations should appear.

Change line 43 in qt/clientmodel.cpp from
connect(pollTimer, SIGNAL(timeout()), this, SLOT(updateTimer()));
to
connect(pollTimer, &QTimer::timeout, this, &ClientModel::updateTimer);

arc lint

Verify that two Qt5 SIGNAL/SLOT violation errors are reported, one for the SIGNAL and one for the SLOT.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR13529
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7900
Build 13810: Bitcoin ABC Buildbot (legacy)
Build 13809: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Oct 29 2019, 20:30
nakihito edited the summary of this revision. (Show Details)

Adjusted a small comment in the lint-qt.sh script.

Fabien requested changes to this revision.Oct 30 2019, 10:42

Please update the diff dependencies according to the parent being splitted.

The script is no longer used since the regex is part of the PHP script. You should remove it and update the comments accordingly.

As discussed offline, is it safe to only check for SIGNAL or SLOT only in order to avoid having 2 errors for the same connect() call ? Is there some case where one could appear and not the other ?

This revision now requires changes to proceed.Oct 30 2019, 10:42

Removed unnecessary script.

Please update the diff dependencies according to the parent being splitted.

The script is no longer used since the regex is part of the PHP script. You should remove it and update the comments accordingly.

As discussed offline, is it safe to only check for SIGNAL or SLOT only in order to avoid having 2 errors for the same connect() call ? Is there some case where one could appear and not the other ?

Script removed and dependency removed (all changes have been landed).
Unfortunately, it is not guaranteed that one will occur with the other. See the following:
SLOT only: https://github.com/bitcoin/bitcoin/pull/13529/commits/f78558f1e39198779bdb17e2b0e256fb99ad4b28#diff-0db7dd184df07a48c307ccc182021a68L959
SLOT only: https://github.com/bitcoin/bitcoin/pull/13529/commits/f78558f1e39198779bdb17e2b0e256fb99ad4b28#diff-bb3be7e8e1a771895eedfe9802528225L21
SIGNAL only: https://github.com/bitcoin/bitcoin/pull/13529/commits/f78558f1e39198779bdb17e2b0e256fb99ad4b28#diff-1bd3480f178e8bf9a556293cc3c6103eL216

Fabien requested changes to this revision.Oct 31 2019, 22:58

One nit, good otherwise.

arcanist/linter/Qt5Linter.php
4 ↗(On Diff #13856)

Needs update

This revision now requires changes to proceed.Oct 31 2019, 22:58

Updated initial comment in Qt5Linter.php.

This revision is now accepted and ready to land.Nov 2 2019, 09:06