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

Add stream operator for miral::Window #2071

Closed
wants to merge 4 commits into from
Closed

Conversation

wmww
Copy link
Contributor

@wmww wmww commented Jun 17, 2021

This makes window's correctly appear in test failures, and may have other uses. google/googletest#1149 explains why the stream operators really need to be declared in the same place as the class. I tried a number of other ways, and this is the only one I found to be successful.

@@ -78,6 +79,7 @@ bool operator==(Window const& lhs, Window const& rhs);
bool operator==(std::shared_ptr<mir::scene::Surface> const& lhs, Window const& rhs);
bool operator==(Window const& lhs, std::shared_ptr<mir::scene::Surface> const& rhs);
bool operator<(Window const& lhs, Window const& rhs);
auto operator<<(std::ostream& stream, const miral::Window& window) -> std::ostream&;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use right const around here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oop

@@ -125,3 +125,16 @@ bool miral::operator<(Window const& lhs, Window const& rhs)
{
return lhs.self.owner_before(rhs.self);
}

auto miral::operator<<(std::ostream& stream, const miral::Window& window) -> std::ostream&
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use right const around here!

Copy link
Collaborator

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

Sure

@AlanGriffiths
Copy link
Collaborator

AlanGriffiths commented Jun 18, 2021

CI say: "You need to add miral::operator<<(std::basic_ostream<char, std::char_traits<char> >&, miral::Window const&) to the libmiral4.symbols"

@wmww
Copy link
Contributor Author

wmww commented Jun 18, 2021

Hmm looks like GitHub is broken

@wmww
Copy link
Contributor Author

wmww commented Jun 19, 2021

I'm going to interpret "Sure" + approval as a bors delegate

bors r=AlanGriffiths

bors bot added a commit that referenced this pull request Jun 19, 2021
2071: Add stream operator for miral::Window r=AlanGriffiths a=wmww

This makes window's correctly appear in test failures, and may have other uses. google/googletest#1149 explains why the stream operators really need to be declared in the same place as the class. I tried a number of other ways, and this is the only one I found to be successful.

Co-authored-by: William Wold <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jun 19, 2021

Build failed:

@AlanGriffiths
Copy link
Collaborator

I'm going to interpret "Sure" + approval as a bors delegate

Fair enough. (I'm not really convinced this is a widely useful streaming operator, nor that it has to be declared outside the test framework, but it is relatively harmless.)

@wmww
Copy link
Contributor Author

wmww commented Jun 23, 2021

I'm not really convinced this is a widely useful streaming operator, nor that it has to be declared outside the test framework

I can't say it has to be, but I put a bit of work into trying different things and this is the only one I found to actually work.

bors r=AlanGriffiths

bors bot added a commit that referenced this pull request Jun 23, 2021
2071: Add stream operator for miral::Window r=AlanGriffiths a=wmww

This makes window's correctly appear in test failures, and may have other uses. google/googletest#1149 explains why the stream operators really need to be declared in the same place as the class. I tried a number of other ways, and this is the only one I found to be successful.

Co-authored-by: William Wold <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jun 23, 2021

Build failed:

@AlanGriffiths
Copy link
Collaborator

OK, so the reason this is problematic is that in the MIRAL_3.0 stanza of the version script (symbols.map) there's a wildcard that matches the new operator:

    miral::operator*;

Currently, that corresponds to:

miral::operator<(miral::Window const&, miral::Window const&)
miral::operator==(miral::Output::PhysicalSizeMM const&, miral::Output::PhysicalSizeMM const&)
miral::operator==(miral::Window const&, miral::Window const&)
miral::operator==(miral::Window const&, std::shared_ptr<mir::scene::Surface> const&)
miral::operator==(std::shared_ptr<mir::scene::Surface> const&, miral::Window const&)

So these need to be pulled in unambiguously (e.g. by using the mangled names).

@wmww
Copy link
Contributor Author

wmww commented Jun 24, 2021

@RAOF Alan mentioned to may know some secret of escaping characters in symbols.map?

Copy link
Collaborator

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

Having poked around a bit I can't find a simple way to find this operator at the point of instantiation.

OTOH I think it would be better to provide a PrintTo() function for gtest, not a streaming operator that might be confused with something useful. That is, in miral/window.h:

// For Google test
void PrintTo(Window const& window, std::ostream* out);

@AlanGriffiths
Copy link
Collaborator

Closing in favour of #2081

@AlanGriffiths AlanGriffiths deleted the miral-window-stream branch June 25, 2021 14:27
bors bot added a commit that referenced this pull request Jun 25, 2021
2081: Add gtest customization to print surface names in error messages r=wmww a=AlanGriffiths

Better solution than #2071

Co-authored-by: Alan Griffiths <[email protected]>
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