namecoin / namecoin-core

Namecoin full node + wallet based on the current Bitcoin Core codebase.
https://www.namecoin.org/
MIT License
457 stars 146 forks source link

Missing #include in buynamespage.h #520

Closed ghost closed 1 year ago

ghost commented 1 year ago

Both Namecoin Core 23.0 (https://github.com/namecoin/namecoin-core/issues/515) and 24.0, as shown below, fail to compile due to a missing #include.

This pull request resolves the issue in Namecoin Core 24.0 and subsequent releases.

In file included from qt/buynamespage.cpp:1:
./qt/buynamespage.h:34:57: error: ‘optional’ in namespace ‘std’ does not name a template type
   34 |     QString firstupdate(const QString &name, const std::optional<QString> &value, const std::optional<QString> &transferTo) const;
      |                                                         ^~~~~~~~
./qt/buynamespage.h:7:1: note: ‘std::optional’ is defined in header ‘<optional>’; did you forget to ‘#include <optional>’?
    6 | #include <QWidget>
  +++ |+#include <optional>
    7 | 
./qt/buynamespage.h:34:65: error: expected ‘,’ or ‘...’ before ‘<’ token
   34 |     QString firstupdate(const QString &name, const std::optional<QString> &value, const std::optional<QString> &transferTo) const;
      |                                                                 ^
./qt/buynamespage.h:37:10: warning: ‘virtual bool BuyNamesPage::eventFilter(QObject*, QEvent*)’ can be marked override [-Wsuggest-override]
   37 |     bool eventFilter(QObject *object, QEvent *event);
      |          ^~~~~~~~~~~
qt/buynamespage.cpp: In member function ‘void BuyNamesPage::onRegisterNameAction()’:
qt/buynamespage.cpp:95:46: error: no matching function for call to ‘BuyNamesPage::firstupdate(QString&, const QString&, const std::optional<QString>&)’
   95 |     const QString err_msg = this->firstupdate(name, newValue, transferToAddress);
      |                             ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./qt/buynamespage.h:34:13: note: candidate: ‘QString BuyNamesPage::firstupdate(const QString&, int) const’
   34 |     QString firstupdate(const QString &name, const std::optional<QString> &value, const std::optional<QString> &transferTo) const;
      |             ^~~~~~~~~~~
./qt/buynamespage.h:34:13: note:   candidate expects 2 arguments, 3 provided
qt/buynamespage.cpp: At global scope:
qt/buynamespage.cpp:142:9: error: no declaration matches ‘QString BuyNamesPage::firstupdate(const QString&, const std::optional<QString>&, const std::optional<QString>&) const’
  142 | QString BuyNamesPage::firstupdate(const QString &name, const std::optional<QString> &value, const std::optional<QString> &transferTo) const
      |         ^~~~~~~~~~~~
./qt/buynamespage.h:34:13: note: candidate is: ‘QString BuyNamesPage::firstupdate(const QString&, int) const’
   34 |     QString firstupdate(const QString &name, const std::optional<QString> &value, const std::optional<QString> &transferTo) const;
      |             ^~~~~~~~~~~
./qt/buynamespage.h:18:7: note: ‘class BuyNamesPage’ defined here
   18 | class BuyNamesPage : public QWidget
      |       ^~~~~~~~~~~~
make[2]: *** [Makefile:12660: qt/libbitcoinqt_a-buynamespage.o] Error 1
make[2]: Leaving directory '/home/hacker/namecoin-core-nc24.0/src'
make[1]: *** [Makefile:19356: all-recursive] Error 1
make[1]: Leaving directory '/home/hacker/namecoin-core-nc24.0/src'
make: *** [Makefile:823: all-recursive] Error 1
domob1812 commented 1 year ago

Looks good to me (although I think the main issue is on 0.24, so we might want to fix on there afterwards). There is an #include <optional> in buynamespage.cpp already on master. However, since the header also uses std::optional, I agree that the include should be there. Please remove the one in the cpp file with this PR, then it can be merged.

ghost commented 1 year ago

@domob1812

I removed #include <optional> from buynamespage.cpp as you requested and successfully compiled Namecoin Core from source with the changes in this pull request.

ghost commented 1 year ago

@domob1812

You can do it yourself by using the merge pull request button's drop-down menu.

JeremyRand commented 1 year ago

ACK 006fb0f8182364068fd108bae17d09334b4a24be, subject to squashing. Didn't test it but the change looks reasonable (and I have no idea why it built for some platforms without this).

@domob1812 Reminder to please include the commit hash in ACK messages.

@redarmyfaction Namecoin Core follows Bitcoin Core's security policy of not using GitHub merge commits, so that won't be an option. If you're not able to squash it yourself, I'm happy to do it for you, let me know.

ghost commented 1 year ago

@domob1812 @jeremyrand

This pull request is complete and ready for merging.

In addition, https://github.com/namecoin/namecoin-core/issues/515 has been resolved and can be closed.

domob1812 commented 1 year ago

@redarmyfaction Please squash all commits into a single one as stated previously.

ghost commented 1 year ago

@domob1812 Done.

JeremyRand commented 1 year ago

ACK 53a0fe5181b0c35bbfc2e268209bcd7e90d99642