Compiler warning C4101: unreferenced local variable #4856

ENikS commented at 3:46 PM on September 5, 2014: contributor

Another attempt at fixing this warning.

Fixing compiler warning C4101 4c3bd0bb6e

in src/core_read.cpp:None in 4c3bd0bb6e

  96 | @@ -97,7 +97,7 @@ bool DecodeHexTx(CTransaction& tx, const std::string& strHexTx)
  97 |      try {
  98 |          ssData >> tx;
  99 |      }
 100 | -    catch (std::exception &e) {
 101 | +    catch (std::exception const &) {

laanwj commented at 3:56 PM on September 5, 2014:

If your intent is to make it a const reference, shouldn't it at least be const std::exception &? Otherwise you're making the reference itself const but references are const by definition (you can't change what they point to). Or am I missing something? Why the const in the first place?

ENikS commented at 4:18 PM on September 5, 2014:

http://www.parashift.com/c++-faq-lite/const-ref-alt.html

laanwj commented at 4:23 PM on September 5, 2014:

Ah, right, I was confused with std::exception & const, which is nonsense.

ENikS commented at 4:30 PM on September 5, 2014:

What is nonsense, std::exception or std::exception & const?

laanwj commented at 4:33 PM on September 5, 2014:

Putting the const at the end. That literally comes from the link that you posted and is also what I meant above with "otherwise you're making the reference itself const but references are const by definition":

Another caveat: if you decide to use X const&, do something to make sure your people don't mis-type it as the nonsensical "X& const x".

BitcoinPullTester commented at 4:00 PM on September 5, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4856_4c3bd0bb6e1c7e22c381802f6bfd6a60a905f637/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

sipa commented at 3:46 PM on September 6, 2014: member

Untested ACK

TheBlueMatt commented at 4:04 AM on September 8, 2014: member

Does bitcoin use the T const & syntax elsewhere? seems our style is const T&, or does that not fix the warning?

laanwj commented at 7:26 AM on September 8, 2014: member

@TheBlueMatt That would solve the warning just as well.

ENikS commented at 3:03 PM on September 8, 2014: contributor

@TheBlueMatt

Global search reveals that 'const T' has been used exactly two times. So I would say majority of the code (9 files) uses 'T const'.

Find all "catch._(._const", Regular expressions, Subfolders, Find Results 1, Entire Solution, D:\bitcoin-dev-msvc\bitcoin\src\walletdb.cpp(880): } catch(const filesystem::filesystem_error &e) { D:\bitcoin-dev-msvc\bitcoin\src\compat\glibcxx_sanity.cpp(54): catch (const std::out_of_range&) Matching lines: 2 Matching files: 2 Total files searched: 510

But seriously, unless you pay me salary you should not demand changing perfectly normal code just because it does not comply with your cosmetic preferences.

sipa commented at 3:19 PM on September 8, 2014: member

Your assumption is incorrect. There are only two cases whatsoever where an exception is caught in a const variable. Both of these have the 'const' in front.

In fact, I believe every const variable has the const in front ("const &" or "const&" appear nowhere in the source tree). So, yes, please follow the existing style. What you consider 'perfectly normal code' would make our source look inconsistent.

laanwj commented at 3:25 PM on September 8, 2014: member

I could have replaced them 10 times in the time I spent typing replies in this thread; no problem if you don't want to do it, I'll replace them on merge.

sipa commented at 5:37 PM on September 12, 2014: member

Ok, untested ACK apart from coding style :)

jgarzik commented at 1:41 AM on September 15, 2014: contributor

Indeed. Again, new/updated code must look like existing bitcoin code. This is a normal rule for any codebase, anywhere.

ut ACK once code is consistent with our codebase.

laanwj referenced this in commit ec91092df8 on Sep 15, 2014

laanwj commented at 12:36 PM on September 15, 2014: member

Merged via ec91092 (with code style changes)

laanwj closed this on Sep 15, 2014

ENikS deleted the branch on Sep 21, 2014

reddink referenced this in commit 72f541cff1 on May 27, 2020

DrahtBot locked this on Sep 8, 2021