Skip to content

Add fwd declaration headers#1

Closed
strega-nil wants to merge 4 commits intocngzhnp:cngzhnp/fix_forward_declarationsfrom
strega-nil:cngzhnp/fix_forward_declarations
Closed

Add fwd declaration headers#1
strega-nil wants to merge 4 commits intocngzhnp:cngzhnp/fix_forward_declarationsfrom
strega-nil:cngzhnp/fix_forward_declarations

Conversation

@strega-nil
Copy link
Copy Markdown

As opposed to declaring these types at each point of use, add header files for this instead. What are your thoughts @cngzhnp ?

@cngzhnp
Copy link
Copy Markdown
Owner

cngzhnp commented Aug 18, 2020

Interesting way to do! :) Actually, if there is no rule about how to include/order header files and forward declarations on the project, please define one. (clang-format does something but might not be good enough) If it is already defined and this implementation does not break anything at all, seems good to me!

On the other hand, I want to say a few words about it. I'd prefer to keep them on header files because I like include-what-you-use principle.

If you do not want to see repeated forward declarations, there are a few problems with the design. Because when you use them, you try to improve modularity. However, the same forward declarations are done on most of the header files, it might be a design smell. My suggestion is for the next steps, please try to break apart Vcpkgpaths class otherwise these kind of common use will make you have headache or try to implement Pimpl idiom if possible to get rid of things more easily.

TL;DR LGTM, however please consider what I wrote above. Thanks for the request btw!

@cngzhnp
Copy link
Copy Markdown
Owner

cngzhnp commented Aug 19, 2020

@strega-nil Should I merge them into my branch or will you proceed from another pull request?

@strega-nil
Copy link
Copy Markdown
Author

@cngzhnp I proceeded from another pull request.

@cngzhnp cngzhnp closed this Aug 20, 2020
@strega-nil strega-nil deleted the cngzhnp/fix_forward_declarations branch September 10, 2020 21:21
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