Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wallet API: add some missing override keyword #4000

Merged
merged 1 commit into from Jun 28, 2018

Conversation

stoffu
Copy link
Contributor

@stoffu stoffu commented Jun 13, 2018

Also remove dust() from UnsignedTransactionImpl (already in PendingTransactionImpl)

@moneromooo-monero
Copy link
Collaborator

That's a bit spammy, no ?

@stoffu
Copy link
Contributor Author

stoffu commented Jun 14, 2018

Well, this patch was motivated by more than 4000 lines of warning like below:

[ 90%] Building CXX object src/wallet/api/CMakeFiles/obj_wallet_api.dir/wallet.cpp.o
In file included from /Users/stoffu/monero/src/wallet/api/wallet.cpp:32:
/Users/stoffu/monero/src/wallet/api/wallet.h:59:10: warning: 'createWatchOnly' overrides a member function but is not marked
      'override' [-Winconsistent-missing-override]
    bool createWatchOnly(const std::string &path, const std::string &password,
         ^
/Users/stoffu/monero/src/wallet/api/wallet2_api.h:490:18: note: overridden virtual function is here
    virtual bool createWatchOnly(const std::string &path, const std::string &password, const std::string &language) const = 0;
                 ^

all coming from src/wallet/api/wallet.cpp. Full set: https://paste.fedoraproject.org/paste/4Jy5ph89hETLaIC-fl5suA

They seem to be caused by some of the overridden functions in WalletImpl being supplied with the override keyword while others not (the other classes don't have any functions with override and thus don't generate the warnings). However, I noticed that I don't observe this behavior on other platforms. I also don't see it on the logs of the buildbot's OSX builds. Is it just me seeing these warnings?

I'm on macOS Sierra 10.12.6 (16G1408). The compiler version is:

Apple LLVM version 9.0.0 (clang-900.0.39.2)
Target: x86_64-apple-darwin16.7.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

@moneromooo-monero
Copy link
Collaborator

moneromooo-monero commented Jun 14, 2018

This "use it once, use it always" kinda rings a bell, I believe we had this problem earlier with ledger. Alright then, let it go in.

Are you using CLANG by any chance ? I seem to be remembering something to do with it...

@stoffu
Copy link
Contributor Author

stoffu commented Jun 15, 2018

I think I'm using CLANG because that's the default for OSX and I didn't do anything particular to install GCC.

From CMakeCache.txt:

CMAKE_CXX_COMPILER:FILEPATH=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++
CMAKE_C_COMPILER:FILEPATH=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc

Also remove dust() from UnsignedTransactionImpl (already in PendingTransactionImpl)
@luigi1111 luigi1111 merged commit 4510f41 into monero-project:master Jun 28, 2018
luigi1111 added a commit that referenced this pull request Jun 28, 2018
4510f41 Wallet API: add some missing override keyword (stoffu)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants