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

Fix compiler warnings in test directory #302

Merged
merged 5 commits into from
Sep 1, 2023

Conversation

aloisklink
Copy link
Contributor

Fixes a couple of compiler warnings in the test directory.

Since the test directory doesn't affect users, I've just made a single PR that includes all my changes. But please let me know if you'd prefer to break my changes up into smaller PRs (I've tried to split up each change into individual commits, to make it a bit easier to review!)

Copy link
Owner

@skypjack skypjack left a comment

Choose a reason for hiding this comment

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

LGTM but I don't get why the init function was added. Any hints?

@@ -12,4 +12,8 @@ struct fake_stream_handle: uvw::stream_handle<fake_stream_handle, fake_stream_t>
int init(Args &&...) {
return 0;
}

virtual int init() override {
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the slow reviews but I'm out of home for another couple of days.
Can I ask you why did you add this function? I don't see the purpose at first glance. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It prevents the following warning:

src/uvw/uv_type.hpp:32:17: warning: ‘int uvw::uv_type<U>::init() [with U = fake_stream_t]’ was hidden [-Woverloaded-virtual]
   32 |     virtual int init() {
      |                 ^~~~
test/uvw/stream.cpp:12:9: note:   by ‘fake_stream_handle::init(Args&& ...)’
   12 |     int init(Args &&...) override {
                ^~~~

I've added some basic details in the commit description: 628b603.

Some details on why this is a problem is here: https://isocpp.org/wiki/faq/strange-inheritance#hiding-rule

But basically, the template<typename... Args> int init(Args &&...) { return 0;} function doesn't override the virtual init(); function in

uvw/src/uvw/uv_type.hpp

Lines 28 to 34 in 1137725

/**
* @brief Initializes the handle.
* @return Underlying return value.
*/
virtual int init() {
return 0;
}
because it's a template function (however, it does hide the original function). If you want to override it, we need to explicitly declare a non-template version of it.

Alternatively, we could also add a using stream_handle::init; statement, and that should also fix the error, but I'm guessing you're overriding init() for a reason here.


Sorry for the slow reviews

And no worries about the slow reviews :) I help maintain the https:/mermaid-js/mermaid project, and it sometimes takes me weeks to find the time to review really big PRs!

Copy link
Owner

@skypjack skypjack Aug 30, 2023

Choose a reason for hiding this comment

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

I really, REALLY, really feel like init doesn't have to be virtual not in the public interface of the base class actually.
Let me try. If this is the case, I'll push a commit and close this PR without merging it. Gimme a minute. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome! dea293f seems to have fixed this warning!

By the way, what should we do with the other commits in this PR? Would you like me to make separate PRs for each commit?

Alternatively, we could re-open this PR, and we could just drop commit 628b603 if you'd prefer to have one big PR fixing all the warnings in the test/ directory, as although each commit in this PR fixes a separate warning, the test/ directory isn't as important as anything in the src/ directory.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, sorry. I'd reopen this one and strip the commit for the init function.
Can you do that? Otherwise I can port everything to a new branch and merge it directly.
Just let me know. Both are fine for me and sorry again for the mistake on closing this one. 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd reopen this one and strip the commit for the init function.

Done! It was even the last commit, so all the other commits have the same hash.

Otherwise I can port everything to a new branch and merge it directly.

It's up to you, it might be better to do a Rebase and merge instead of a Squash and merge, since each commit is a logically separate change, but it's your GitHub repo, so you can do what you prefer!

Both are fine for me and sorry again for the mistake on closing this one. 🙏

And no worries! I've done it a couple times where I wrote something like, This doesn't fix #xxxx, but GitHub sees fix #xxxx and auto-closes the issue.

@skypjack skypjack self-requested a review August 30, 2023 07:32
@skypjack skypjack closed this in dea293f Aug 30, 2023
@skypjack skypjack reopened this Aug 31, 2023
@aloisklink
Copy link
Contributor Author

I've dropped commit 628b603 as discussed in #302 (comment), since dea293f has already fixed this issue.

I'm not 100% sure why CI is failing, but I'm guessing it might just be a random failure, since everything seems to be working on my aloisklink/uvw fork: aloisklink@b820447. Can you re-run GitHub Actions and see if that fixes it?

@skypjack skypjack merged commit 683883b into skypjack:main Sep 1, 2023
48 checks passed
@aloisklink aloisklink deleted the test-fix-compiler-warnings branch September 1, 2023 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants