[Qt] fix unused function warning in scicon.cpp #6143

pull Diapolo wants to merge 1 commits into bitcoin:master from Diapolo:unused_func changing 1 files +2 −0

Diapolo commented at 7:46 AM on May 15, 2015: none

No description provided.
[Qt] fix unused function warning in scicon.cpp cde9b495f3
paveljanik commented at 8:46 AM on May 15, 2015: contributor

Can you please explain how this could help?
paveljanik commented at 9:16 AM on May 15, 2015: contributor

Hmm, looks like I was looking at the older version of the file ;-) It is clear from the current version. I do not like these #ifdefs all around one file though. But your change really fixes the problem introduced, so ACK.
laanwj commented at 9:56 AM on May 15, 2015: member

I'm not too happy about this. If the code can compile on all platforms the compiler should do it, this makes it less likely that someone will push code that doesn't work on all platforms.

I'm sure there is another way to disable the warning (one is to make the function non-static).
laanwj added the label GUI on May 15, 2015
luke-jr commented at 4:24 PM on May 15, 2015: member

Making it non-static is liable to cause the binaries to be unnecessarily larger too. If we want to disable unused code warnings, I'm sure there's a compiler option for that.
theuni commented at 4:34 PM on May 15, 2015: member

These should really all be runtime checks anyway.

As for making it non-static, anon namespace is the c++ way to handle that. That lets the compiler know that if it's not used in the compilation unit, it's safe to prune.
laanwj commented at 8:42 AM on May 16, 2015: member

Agree that an anonymous namespace is the way to go.
in src/qt/scicon.cpp:None in cde9b495f3
```
  10 | @@ -11,6 +11,7 @@
  11 |  #include <QPalette>
  12 |  #include <QPixmap>
  13 |  
  14 | +#if !defined(WIN32) && !defined(MAC_OSX)
  15 |  static void MakeSingleColorImage(QImage& img, const QColor& colorbase)
  16 |  {
  17 |      img = img.convertToFormat(QImage::Format_ARGB32);
```
jonasschnelli commented at 7:03 AM on May 17, 2015:

What about wrapping L16-L24 with the #ifdefs and remove the ifdef at L30 below? This will probably remove the warning, keep the amounts of ifdefs.

laanwj commented at 6:12 AM on May 18, 2015:

The point is not to reduce the number of ifdefs, but to require the compiler to compile the largest possible share of the code on all platforms.
laanwj commented at 10:07 AM on May 18, 2015: member

@diapolo can you check https://github.com/laanwj/bitcoin/commit/0dab12933f38bebcf175a3356b89536705e05b2b
Diapolo commented at 5:03 PM on May 18, 2015: none

@laanwj Seems like you've got a working patch, feel free to pull that ;).
laanwj referenced this in commit e2e7f9513f on May 19, 2015
laanwj commented at 9:28 AM on May 19, 2015: member

Done via e2e7f9513fc77a4659da0e2bfb8634c1ade3a1bb
laanwj closed this on May 19, 2015
Diapolo deleted the branch on May 19, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels