Page MenuHomePhabricator

Declare single-argument (non-converting) constructors "explicit"

Authored by Fabien on Dec 19 2018, 10:45.



In order to avoid unintended implicit conversions.

Backport of core PR10969
Depends on D2205, D2206 and D2212

Test Plan
make check

Diff Detail

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Dec 19 2018, 10:45
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 19 2018, 10:45
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox requested changes to this revision.Dec 20 2018, 00:38
jasonbcox added a subscriber: jasonbcox.

reverse_iterator.h and qt/callback.h have changes in the backport, but these files do not exist in ABC. Are there backports we need that this depends on?

Also missing explicit on ConnectTrace constructor in validation.cpp

226 ↗(On Diff #6358)

Missed one

250 ↗(On Diff #6358)

Remove this. Looks accidentally added.

39 ↗(On Diff #6358)


103 ↗(On Diff #6358)


42 ↗(On Diff #6358)

nullptr change isn't part of the backport, but looks fine to me

238 ↗(On Diff #6358)


166 ↗(On Diff #6358)

Is there a missing backport for format_error?

720 ↗(On Diff #6358)

This should be explicit. The backport has a similar, but not identical, constructor

This revision now requires changes to proceed.Dec 20 2018, 00:38
Fabien added a comment.Dec 20 2018, 13:38

tinyformat format_error:
The missing backport is PR9963, which addresses the consequences of a format error. IMO this is not a good idea because it does not solve the root issue, the bad format itself (if you remember, some have been fixed recently in addrman). A later PR propose a linter to avoid the format issues which seems by far a better option to me.

The missing backport is PR10098, which fixes QT4 compatibility issues. This is reversed in PR14527 when support for QT4 is dropped, so there is no reason backport PR10098.

The missing backport is PR10193, which is intended to remove BOOST_FOREACH. The method actually used in the codebase is boost::adaptors::reverse. I don't really see any advantage in using a custom reverse_iterator implementation here, as boost is already used everywhere in the code.

226 ↗(On Diff #6358)

This backport is only about single argument constructors

42 ↗(On Diff #6358)

Yes it's part of D2206, which is supposed to be a dependency... I may have missed a rebase

Fabien updated this revision to Diff 6374.Dec 20 2018, 13:41

Fix as per review

Fabien updated this revision to Diff 6383.Dec 21 2018, 11:49

Rebase on top of D2212

deadalnix accepted this revision.Dec 21 2018, 14:47
jasonbcox accepted this revision.Dec 21 2018, 21:35
This revision is now accepted and ready to land.Dec 21 2018, 21:35
This revision was automatically updated to reflect the committed changes.