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

Added missing includes in the general headers. #851

Merged
merged 1 commit into from Apr 2, 2015

Conversation

Projects
None yet
4 participants
@eXpl0it3r
Member

eXpl0it3r commented Mar 30, 2015

I'm not sure what the properties are to include a header, thus I'll just list what isn't included and we can decide if it should get added. Also should we sort the headers lexicographical or "group" them?

System

  • NonCopyable.hpp

Window

  • GlResource.hpp
  • WindowHandle.hpp

Graphics

  • Drawable.hpp
  • PrimitiveType.hpp
  • Rect.hpp
  • RenderTarget.hpp
  • Transformable.hpp

Audio

  • AlResource.hpp
  • SoundSource.hpp

Network

  • Socket.hpp
  • SocketHandle.hpp

@eXpl0it3r eXpl0it3r added this to the 2.3 milestone Mar 30, 2015

@eXpl0it3r eXpl0it3r self-assigned this Mar 30, 2015

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 30, 2015

Member

In my opinion, all headers in the public API should be included from the module headers. That is, not those for dependencies or symbols in sf::priv namespace. It's also not a reason to not include a certain header just because it is indirectly included by another one -- that's an implementation detail.

Lexicographical sorting makes sense... Grouping is probably a bit arbitrary :)

Member

Bromeon commented Mar 30, 2015

In my opinion, all headers in the public API should be included from the module headers. That is, not those for dependencies or symbols in sf::priv namespace. It's also not a reason to not include a certain header just because it is indirectly included by another one -- that's an implementation detail.

Lexicographical sorting makes sense... Grouping is probably a bit arbitrary :)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 31, 2015

Member

So should I add everything?

Personally I'd be fine not to include AlResource.hpp and GlResource.hpp since I don't think they could be used (easily) for anything else than how we use it internally.

Member

eXpl0it3r commented Mar 31, 2015

So should I add everything?

Personally I'd be fine not to include AlResource.hpp and GlResource.hpp since I don't think they could be used (easily) for anything else than how we use it internally.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 31, 2015

Member

You're right. I think the problem is that both classes are currently in the public API although they shouldn't be. Even the documentation of GlResource says "This class is for internal use only". I'd suggest we move both to namespace sf::priv -- or adapt the documentation (in a different PR).

(It doesn't really matter functionality-wise because they're indirectly included as base classes, but we should be consistent with the includes)

Member

Bromeon commented Mar 31, 2015

You're right. I think the problem is that both classes are currently in the public API although they shouldn't be. Even the documentation of GlResource says "This class is for internal use only". I'd suggest we move both to namespace sf::priv -- or adapt the documentation (in a different PR).

(It doesn't really matter functionality-wise because they're indirectly included as base classes, but we should be consistent with the includes)

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 31, 2015

Member

We don't even have to do that. sf::priv is still a publicly accessible namespace. If you really wanted to get GlResource out of there, just add it as a member pointer in all the classes that derive from it now and forward declare it or store it as a void*. Deriving from it isn't even necessary since it doesn't contain any non-static members anyway.

Maybe I might submit a PR doing just this (the same for AlResource), it seems logical and benefits everybody.

Member

binary1248 commented Mar 31, 2015

We don't even have to do that. sf::priv is still a publicly accessible namespace. If you really wanted to get GlResource out of there, just add it as a member pointer in all the classes that derive from it now and forward declare it or store it as a void*. Deriving from it isn't even necessary since it doesn't contain any non-static members anyway.

Maybe I might submit a PR doing just this (the same for AlResource), it seems logical and benefits everybody.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 31, 2015

Member

sf::priv is still a publicly accessible namespace

Yes, but it's explicitly marked as private and not documented. Which implies that we can change anything inside it at any time, and nobody has the right to complain :P

Deriving from it isn't even necessary since it doesn't contain any non-static members anyway.

That's not completely true -- the idea is that default constructor and destructor are invoked automatically, and before/after any members. You would have to do all of that manually with member pointers, defeating RAII and requiring more code. Considering that there is no actual advantage in doing so, I wouldn't recommend it. We don't even save compile time, because the headers are self-contained and tiny.

Member

Bromeon commented Mar 31, 2015

sf::priv is still a publicly accessible namespace

Yes, but it's explicitly marked as private and not documented. Which implies that we can change anything inside it at any time, and nobody has the right to complain :P

Deriving from it isn't even necessary since it doesn't contain any non-static members anyway.

That's not completely true -- the idea is that default constructor and destructor are invoked automatically, and before/after any members. You would have to do all of that manually with member pointers, defeating RAII and requiring more code. Considering that there is no actual advantage in doing so, I wouldn't recommend it. We don't even save compile time, because the headers are self-contained and tiny.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 31, 2015

Member

I don't know what the guideline is behind the sf::priv:: namespace, but doing a text search in the public headers yields that it is only used in forward declarations of m_impl types. @LaurentGomila and @mantognini, can you shed some light on the sf::priv:: namespace and how it was intended to be used?

Member

binary1248 commented Mar 31, 2015

I don't know what the guideline is behind the sf::priv:: namespace, but doing a text search in the public headers yields that it is only used in forward declarations of m_impl types. @LaurentGomila and @mantognini, can you shed some light on the sf::priv:: namespace and how it was intended to be used?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 31, 2015

Member

Added the other headers. Feel free to say if something needs to be changed or set the "accepted" flag otherwise. 😉

Member

eXpl0it3r commented Mar 31, 2015

Added the other headers. Feel free to say if something needs to be changed or set the "accepted" flag otherwise. 😉

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 1, 2015

Member

If you don't know how to handle the sf::priv namespace, and there's no particular issue with it, I'd say... don't change anything. About the original question, I'm not strict about it but my feeling is that private headers should not appear in module headers, although they are part of the distributed SFML headers. Only end-user classes should appear there.

Member

LaurentGomila commented Apr 1, 2015

If you don't know how to handle the sf::priv namespace, and there's no particular issue with it, I'd say... don't change anything. About the original question, I'm not strict about it but my feeling is that private headers should not appear in module headers, although they are part of the distributed SFML headers. Only end-user classes should appear there.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Apr 1, 2015

Member

I'm not strict about it but my feeling is that private headers should not appear in module headers

That's one of the issues I have. Which ones are private headers?

Member

eXpl0it3r commented Apr 1, 2015

I'm not strict about it but my feeling is that private headers should not appear in module headers

That's one of the issues I have. Which ones are private headers?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 1, 2015

Member

And why are private headers still documented? 😛

In Thor, I created sub-directories called "Detail" for private headers (e.g. here). They also contain .inl files for templates. I told Doxygen to exclude those directories, so the headers won't appear in the public API documentation... Maybe something similar could be done for SFML.

Member

Bromeon commented Apr 1, 2015

And why are private headers still documented? 😛

In Thor, I created sub-directories called "Detail" for private headers (e.g. here). They also contain .inl files for templates. I told Doxygen to exclude those directories, so the headers won't appear in the public API documentation... Maybe something similar could be done for SFML.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 1, 2015

Member

That's one of the issues I have. Which ones are private headers?

I thought we were talking about sf::priv, but GlResource is not in this namespace. Well, then we're talking about stuff which is marked as internal 😁

And why are private headers still documented?

Documentation is not only for end users.

Member

LaurentGomila commented Apr 1, 2015

That's one of the issues I have. Which ones are private headers?

I thought we were talking about sf::priv, but GlResource is not in this namespace. Well, then we're talking about stuff which is marked as internal 😁

And why are private headers still documented?

Documentation is not only for end users.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Apr 1, 2015

Member

Well I still don't know what the general stance on this PR is, but I guess I'll just add my usual sentence then.

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Apr 1, 2015

Well I still don't know what the general stance on this PR is, but I guess I'll just add my usual sentence then.

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r added s:accepted and removed s:undecided labels Apr 1, 2015

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Apr 1, 2015

Member

Let's put it this way: People (both us and the users) aren't going to lose any sleep over whether GlResource and AlResource are part of those headers or in sf::priv:: or not, so whichever you choose, it really doesn't matter that much. We have more important issues to worry about. 😉

Member

binary1248 commented Apr 1, 2015

Let's put it this way: People (both us and the users) aren't going to lose any sleep over whether GlResource and AlResource are part of those headers or in sf::priv:: or not, so whichever you choose, it really doesn't matter that much. We have more important issues to worry about. 😉

@eXpl0it3r eXpl0it3r merged commit 3d0ab05 into master Apr 2, 2015

@eXpl0it3r eXpl0it3r deleted the bugfix/general_headers branch Apr 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment