[util] CopyrightHolders: Check for untranslated substitution #8291

MarcoFalke commented at 9:31 PM on June 29, 2016: member

MarcoFalke added the label Utils and libraries on Jun 29, 2016

MarcoFalke added this to the milestone 0.13.0 on Jun 29, 2016

in src/util.cpp:None in fa9eaa9a29 outdated

 805 | -    if (strCopyrightHolders.find("%s") != strCopyrightHolders.npos) {
 806 | -        strCopyrightHolders = strprintf(strCopyrightHolders, _(COPYRIGHT_HOLDERS_SUBSTITUTION));
 807 | -    }
 808 | -    if (strCopyrightHolders.find("Bitcoin Core developers") == strCopyrightHolders.npos) {
 809 | +    std::string strCopyrightHolders = strPrefix + strprintf(_(COPYRIGHT_HOLDERS), _(COPYRIGHT_HOLDERS_SUBSTITUTION));
 810 | +    if (std::string("Bitcoin Core").compare(COPYRIGHT_HOLDERS_SUBSTITUTION) != 0) {

laanwj commented at 8:38 AM on June 30, 2016:

or just: if (std::string("Bitcoin Core") != COPYRIGHT_HOLDERS_SUBSTITUTION) No need to use compare() if you don't need to know the relative sorting.

laanwj commented at 8:38 AM on June 30, 2016: member

Yes I think this is an improvement, utACK

MarcoFalke force-pushed on Jun 30, 2016

in src/util.cpp:None in fa7a206990 outdated

 801 | @@ -802,10 +802,11 @@ int GetNumCores()
 802 |  std::string CopyrightHolders(const std::string& strPrefix)
 803 |  {
 804 |      std::string strCopyrightHolders = strPrefix + _(COPYRIGHT_HOLDERS);
 805 | -    if (strCopyrightHolders.find("%s") != strCopyrightHolders.npos) {
 806 | +    if (strCopyrightHolders.find("%s") != std::string::npos) {
 807 |          strCopyrightHolders = strprintf(strCopyrightHolders, _(COPYRIGHT_HOLDERS_SUBSTITUTION));
 808 |      }
 809 | -    if (strCopyrightHolders.find("Bitcoin Core developers") == strCopyrightHolders.npos) {
 810 | +    if (std::string("Bitcoin Core") != COPYRIGHT_HOLDERS_SUBSTITUTION) {

roques commented at 9:56 AM on July 1, 2016:

Why not if ("Bitcoin Core" != COPYRIGHT_HOLDERS_SUBSTITUTION) {? At least g++(1) is able to evaluate that during compilation. When using std::string it generates code to create a std::string object and then calls the comparison function on that. The right hand side of the comparison is a "…" and not a std::string anyway. Or is there a style-reason for using std::string? This code is definitely neither size- nor speed-critical.

MarcoFalke commented at 10:18 AM on July 1, 2016:

I have no background in cpp but the compiler tells me on your suggestion

util.cpp: In function ‘std::__cxx11::string CopyrightHolders(const string&)’:
util.cpp:805:27: warning: comparison with string literal results in unspecified behaviour [-Waddress]
     if ("Bitcoin Core" != COPYRIGHT_HOLDERS_SUBSTITUTION) {
                           ^

laanwj commented at 10:33 AM on July 1, 2016:

Noooo please keep the std::string, comparing char*'s will not do what you want it to.

sipa commented at 10:37 AM on July 1, 2016:

Comparing two string literals will likely do what you want due to the compiler merging the constants to the same memory location. This is not behaviour to rely on though.

roques commented at 11:18 AM on July 1, 2016:

@sipa, definitely. Please, don't do what I suggested. if (strcmp("Bitcoin Core", COPYRIGHT_HOLDERS_SUBSTITUTION) != 0) can also be evaluated at compile time, but that's C, not C++ and is harder to read than what the patch currently does. Thus, just forget my suggestion.

laanwj commented at 11:33 AM on July 1, 2016:

Comparing two string literals will likely do what you want due to the compiler merging the constants to the same memory location. This is not behaviour to rely on though.

LOL yes 'it will mostly work' is the worst kind of working.

if (strcmp("Bitcoin Core", COPYRIGHT_HOLDERS_SUBSTITUTION) != 0) can also be evaluated at compile time

I don't think we care here, it's not like this is a performance critical inner loop.

[util] CopyrightHolders: Check for untranslated substitution

Also, remove check which is always true

33336e1aac

in src/util.cpp:None in fa97c3d40c outdated

 800 | @@ -801,11 +801,9 @@ int GetNumCores()
 801 |  
 802 |  std::string CopyrightHolders(const std::string& strPrefix)
 803 |  {
 804 | -    std::string strCopyrightHolders = strPrefix + _(COPYRIGHT_HOLDERS);
 805 | -    if (strCopyrightHolders.find("%s") != strCopyrightHolders.npos) {
 806 | -        strCopyrightHolders = strprintf(strCopyrightHolders, _(COPYRIGHT_HOLDERS_SUBSTITUTION));
 807 | -    }
 808 | -    if (strCopyrightHolders.find("Bitcoin Core developers") == strCopyrightHolders.npos) {
 809 | +    std::string strCopyrightHolders = strPrefix + strprintf(_(COPYRIGHT_HOLDERS), _(COPYRIGHT_HOLDERS_SUBSTITUTION));

luke-jr commented at 4:49 PM on July 1, 2016:

This will break when COPYRIGHT_HOLDERS might not have "%s" to be replaced.

MarcoFalke commented at 8:49 AM on July 2, 2016:

I don't think we support COPYRIGHT_HOLDERS with no "%s" in it. (If you really wanted to do that you'd also have to assert that COPYRIGHT_HOLDERS_SUBSTITUTION is empty? etc)

If someone feels like breaking that, they can figure out a fix for themselves. There is no need to keep unused code in our repo because someday someone might or might not use it.

laanwj commented at 7:39 AM on July 7, 2016:

@luke-jr In that case strprintf will explode in your face But I don't see that as a problem, really. We support with %s, if you really want to do something differently you have to change this code too.

MarcoFalke commented at 7:52 AM on July 7, 2016:

It will only explode during run time, but I still think we should not keep fancy logic and several unused lines of code.

MarcoFalke commented at 6:08 PM on July 15, 2016:

@luke-jr Ideally there should be documentation (and commented out sample code) in configure.ac about the preferred method of adding a copyright string. Adding some heuristics to detect such additional string in some downstream helper method is good intention but doesn't really help, imo.

laanwj commented at 8:53 AM on July 18, 2016:

Adding some heuristics to detect such additional string in some downstream helper method is good intention but doesn't really help, imo. Add a line note

The point here was to make it impossible to get rid of our copyright by just changing a build system variable. It feels incredibly brittle if all of the copyright string hinges on is something that is passed to the compiler as -D define.

Agree with adding documentation of course, but this heuristic should stay.

MarcoFalke commented at 9:01 AM on July 18, 2016:

I think with heuristic I meant "try to handle strings with no %s correctly"

laanwj commented at 9:12 AM on July 18, 2016:

Agree in that case.

in src/util.cpp:None in fa97c3d40c outdated

 805 | -    if (strCopyrightHolders.find("%s") != strCopyrightHolders.npos) {
 806 | -        strCopyrightHolders = strprintf(strCopyrightHolders, _(COPYRIGHT_HOLDERS_SUBSTITUTION));
 807 | -    }
 808 | -    if (strCopyrightHolders.find("Bitcoin Core developers") == strCopyrightHolders.npos) {
 809 | +    std::string strCopyrightHolders = strPrefix + strprintf(_(COPYRIGHT_HOLDERS), _(COPYRIGHT_HOLDERS_SUBSTITUTION));
 810 | +    if (std::string("Bitcoin Core") != COPYRIGHT_HOLDERS_SUBSTITUTION) {

luke-jr commented at 4:50 PM on July 1, 2016:

Bitcoin Core developers could very well be mentioned in COPYRIGHT_HOLDERS rather than COPYRIGHT_HOLDERS_SUBSTITUTION. For example:

 define(_COPYRIGHT_HOLDERS,[The %s and Bitcoin Core developers])
 define(_COPYRIGHT_HOLDERS_SUBSTITUTION,[Altcoin Core])

MarcoFalke force-pushed on Jul 2, 2016

MarcoFalke removed this from the milestone 0.13.0 on Jul 6, 2016

MarcoFalke commented at 9:13 AM on July 18, 2016: member

Test 1:

 define(_COPYRIGHT_YEAR, 2016)
-define(_COPYRIGHT_HOLDERS,[The %s developers])
+define(_COPYRIGHT_HOLDERS,[The %s and other developers])
 define(_COPYRIGHT_HOLDERS_SUBSTITUTION,[[Bitcoin Core]])

Result 1: :white_check_mark:

 src/bitcoind -version | head -2
Bitcoin Core Daemon version v0.12.99.0-33336e1-dirty
Copyright (C) 2009-2016 The Bitcoin Core and other developers

Test 2:

 define(_COPYRIGHT_YEAR, 2016)
-define(_COPYRIGHT_HOLDERS,[The %s developers])
-define(_COPYRIGHT_HOLDERS_SUBSTITUTION,[[Bitcoin Core]])
+define(_COPYRIGHT_HOLDERS,[The %s and other developers])
+define(_COPYRIGHT_HOLDERS_SUBSTITUTION,[[Bitcoin Core and Other Core]])
 AC_INIT([Bitcoin Core],[_CLIENT_VERSION_MAJOR._CLIENT_VERSION_MINOR._CLIENT_VERSION_REVISION],[https://github.com/bitcoin/bitcoin/issues],[bitcoin],[https://bitcoincore.org/])

Result 2: :white_check_mark:

src/bitcoind -version | head -3
Bitcoin Core Daemon version v0.12.99.0-33336e1-dirty
Copyright (C) 2009-2016 The Bitcoin Core and Other Core and other developers

Test 3:

 define(_COPYRIGHT_YEAR, 2016)
-define(_COPYRIGHT_HOLDERS,[The %s developers])
+define(_COPYRIGHT_HOLDERS,[The other developers])
 define(_COPYRIGHT_HOLDERS_SUBSTITUTION,[[Bitcoin Core]])

Result 3: :white_check_mark:

src/bitcoind -version | head -3
terminate called after throwing an instance of 'std::runtime_error'
  what():  tinyformat: Not enough conversion specifiers in format string

MarcoFalke added the label Needs backport on Jul 18, 2016

MarcoFalke added this to the milestone 0.13.1 on Aug 17, 2016

laanwj merged this on Aug 31, 2016

laanwj closed this on Aug 31, 2016

laanwj commented at 2:17 PM on August 31, 2016: member

utACK 33336e1

laanwj referenced this in commit 5cac8b123e on Aug 31, 2016

MarcoFalke removed the label Needs backport on Sep 9, 2016

MarcoFalke deleted the branch on Sep 9, 2016

codablock referenced this in commit 8435c10b2e on Sep 19, 2017

codablock referenced this in commit 3f47d40508 on Jan 9, 2018

codablock referenced this in commit 5ce7ba6115 on Jan 9, 2018

andvgal referenced this in commit 4ea4d69893 on Jan 6, 2019

MarcoFalke locked this on Sep 8, 2021