move most explicit getters in optionsmodel to header #1900

pull Diapolo wants to merge 1 commits into bitcoin:master from Diapolo:optionsmodel_getters changing 2 files +4 −24

Diapolo commented at 11:39 AM on October 2, 2012: none
- is more consistent and saves quite some lines of code
laanwj commented at 12:06 PM on October 2, 2012: member

OK to the changes, except the one that requires an "extern" global variable in the header file.

The idea is to keep the communication with the bitcoin core limited to the *model.cpp implementation files.
Diapolo commented at 12:27 PM on October 2, 2012: none

That one is easy to revert, I wasn't aware of the reason you mentioned above. Edit: Updated!
laanwj commented at 12:35 PM on October 2, 2012: member

It's violated in a few places, for example signmessage.cpp directly uses core calls and data structures. That's very low-priority, and it's not worth adding too much code for, but when I can I try to prevent it.
move most explicit getters in optionsmodel to header
- is more consistent and saves quite some lines of code
7bc65ff108
in src/qt/optionsmodel.h:None in cff06b058b outdated
```
   0 | @@ -1,6 +1,8 @@
   1 |  #ifndef OPTIONSMODEL_H
   2 |  #define OPTIONSMODEL_H
   3 |  
   4 | +#include "uint256.h"
```
laanwj commented at 4:27 PM on October 2, 2012:

That one can go, too.

Diapolo commented at 4:50 PM on October 2, 2012:

Indeed, I forgot to remove it while reverting the getTransactionFee() change, thanks for noticing!
Diapolo commented at 4:52 PM on October 2, 2012: none

@laanwj Thanks for explaining the idea to limit core access to *model.cpp, but I have to ask what makes the difference between having them in a .cpp or a .h only in the end?
laanwj commented at 5:22 PM on October 2, 2012: member

Compilation time, and it makes it easier to separate out the UI code, for example if we make the UI a separate process.
BitcoinPullTester commented at 2:40 PM on October 6, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/7bc65ff1083c726391923fddac86e5abc4b0f2db for binaries and test log.
sipa commented at 1:03 AM on October 7, 2012: member

Any reason why having the getters in .h is preferrable?
laanwj commented at 7:09 AM on October 7, 2012: member

I'm neutral on this. This change is balancing on the ugly edges of C++, on one side this cuts down on the amount of boilerplate lines to compensate for the lack of properties, on the other hand this moves implementation details to the interface description which is also undesirable (but in many cases unavoidable, at least if you want to make full use of the language with templates and such).
sipa commented at 12:29 PM on October 7, 2012: member

@laanwj Ok, good to know. There has been a general trend towards moving code to .cpp files, but I wasn't sure to what extent we want to pursue this. I don't care about having such oneliners in .h files.
laanwj commented at 12:36 PM on October 7, 2012: member

Right, in the case of non-type-parametrized and non-trivial code, the implementation should certainly be in the implementation file.
Diapolo commented at 4:04 PM on October 7, 2012: none

I found it rather hard to follow your discussion as I'm missing some tech-english here. In the end is such a change wanted or should it be avoided (ACK / NACK)?

Edit: And as a remainder, the getLanguage() function was already in the header :).
sipa commented at 4:06 PM on October 7, 2012: member

Yes, ACK.
laanwj commented at 4:15 PM on October 7, 2012: member

ACK
laanwj referenced this in commit fae3989ffc on Oct 11, 2012
laanwj merged this on Oct 11, 2012
laanwj closed this on Oct 11, 2012
KolbyML referenced this in commit aafd0541c2 on Dec 5, 2020
DrahtBot locked this on Sep 8, 2021

Contributors