script: Disallow silent bool -> CScript conversion #18621

1. 
   MarcoFalke commented at 12:57 pm on April 13, 2020: member

   Makes nonsensical stuff like ScriptToAsmStr(false,false); a compile failure
2. script: Disallow silent bool -> CScript conversion 88884ee8d8
3. 
   laanwj commented at 1:02 pm on April 13, 2020: member

   ACK 88884ee8d8dcd5303b20e54801b03f9631959c76
4. DrahtBot added the label Consensus on Apr 13, 2020
5. 
   promag commented at 1:57 pm on April 13, 2020: member

   ACK 88884ee8d8dcd5303b20e54801b03f9631959c76.
6. 
   DrahtBot commented at 4:37 pm on April 13, 2020: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #18612 (script: Remove undocumented and unused operator+ by MarcoFalke)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

7. 
   practicalswift commented at 5:11 pm on April 13, 2020: contributor

   ACK 88884ee8d8dcd5303b20e54801b03f9631959c76

   explicit is better than implicit.

   More generally I think we should err on the safe side and only allow implicit conversions via explicit opt-in (instead of disallowing via explicit opt-out) :)

8. jb55 approved
9. 
   jb55 commented at 5:14 pm on April 13, 2020: member

   ACK 88884ee8d8dcd5303b20e54801b03f9631959c76
10. ryanofsky approved
11. 
    ryanofsky commented at 5:26 pm on April 13, 2020: member

    Code review ACK 88884ee8d8dcd5303b20e54801b03f9631959c76

    I think it’s not possible for just marking a constructor explicit to change which overload a compiler chooses like sipa was speculating about in https://github.com/bitcoin/bitcoin/pull/18612/files#r407269834 (assuming no sfinae overloads). But might be worth verifying this PR doesn’t change compiler output.

12. 
    MarcoFalke commented at 5:53 pm on April 13, 2020: member

    But might be worth verifying this PR doesn’t change compiler output.

    I get the same bin with gcc/clang on O2

13. 
    sipa commented at 6:27 pm on April 13, 2020: member

    @ryanofsky FWIW, that is not true in general.

    The following code

     0#include <stdio.h>
     1
     2struct A { };
     3struct B : public A { };
     4struct C {
     5    C(const A&) {printf("A\n");}
     6    C(const B&) {printf("B\n");}
     7};
     8
     9int main(void) {
    10    const B objb = B();
    11    const C objc = objb;
    12
    13    return 0;
    14}
    

    will print B. When the C::C(const B&) constructor is marked explicit, it prints A. So it seems that:

    • Explicit constructors only match in explicit contexts.
    • deleted functions/constructors match just as much as non-deleted ones.
    • Among all matching functions the most specific one is chosen.
    • If the most specific one is deleted, there is an error.
14. 
    instagibbs commented at 5:22 pm on April 14, 2020: member

    utACK 88884ee8d8dcd5303b20e54801b03f9631959c76
15. 
    ryanofsky commented at 6:14 pm on April 14, 2020: member

    @ryanofsky FWIW, that is not true in general.

    Thanks, makes sense! I was thinking implicit constructor conversions would be pretty low in precedence but it makes sense derived-to-base conversions are lower

16. fanquake merged this on Apr 15, 2020
17. fanquake closed this on Apr 15, 2020

    ---

18. MarcoFalke deleted the branch on Apr 15, 2020
19. sidhujag referenced this in commit 9376e7f339 on Apr 15, 2020
20. ComputerCraftr referenced this in commit 0d51754852 on Jun 10, 2020
21. Fabcien referenced this in commit 4f308eaa81 on Jan 18, 2021
22. PastaPastaPasta referenced this in commit a2c2ca980b on Jun 27, 2021
23. PastaPastaPasta referenced this in commit 0ab09c07c2 on Jun 28, 2021
24. PastaPastaPasta referenced this in commit 05384cd2c2 on Jun 29, 2021
25. PastaPastaPasta referenced this in commit b756c19fd0 on Jul 1, 2021
26. PastaPastaPasta referenced this in commit 49e93b17d2 on Jul 1, 2021
27. PastaPastaPasta referenced this in commit 71d4883643 on Jul 14, 2021
28. PastaPastaPasta referenced this in commit 7b74287215 on Jul 14, 2021
29. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke laanwj promag DrahtBot practicalswift jb55 ryanofsky sipa instagibbs

Labels
Consensus