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

Missing #include in buynamespage.h #520

Merged
merged 1 commit into from Jan 6, 2023
Merged

Missing #include in buynamespage.h #520

merged 1 commit into from Jan 6, 2023

Conversation

ghost
Copy link

@ghost ghost commented Dec 19, 2022

Both Namecoin Core 23.0 (#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
Copy link

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
Copy link
Author

ghost commented Dec 20, 2022

@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.

Copy link

@domob1812 domob1812 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Please squash the commits into a single one, then this can be merged.

@ghost
Copy link
Author

ghost commented Dec 22, 2022

@domob1812

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

@JeremyRand
Copy link
Member

ACK 006fb0f, 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
Copy link
Author

ghost commented Dec 24, 2022

@domob1812 @JeremyRand

This pull request is complete and ready for merging.

In addition, #515 has been resolved and can be closed.

@domob1812
Copy link

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

@ghost
Copy link
Author

ghost commented Jan 2, 2023

@domob1812 Done.

@JeremyRand
Copy link
Member

ACK 53a0fe5

@JeremyRand JeremyRand merged commit df9da13 into namecoin:master Jan 6, 2023
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.

2 participants