- Notifications
You must be signed in to change notification settings - Fork147
DLL-inteface for operator<< (MSVC compatibility)#114
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In general, I preferfunctional changes to be split into a separate commit fromformatting or style changes.
docopt.cpp Outdated
| @@ -21,10 +21,10 @@ | |||
| #include <cassert> | |||
| #include <cstddef> | |||
| usingnamespace docopt; | |||
| namespace docopt { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you explain why you're doing this? I don't necessarily oppose it but would like to understand the justification. (Especially because you labeled this as something to do with GCC, which shouldn't be an issue).
One reason I like the "docopt::" in the definitions is that it catches bugs (if a function gets defined that didn't have a declaration).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, use previous style
| @@ -67,7 +43,8 @@ namespace docopt { | |||
| /// @throws DocoptExitHelp if 'help' is true and the user has passed the '--help' argument | |||
| /// @throws DocoptExitVersion if 'version' is true and the user has passed the '--version' argument | |||
| /// @throws DocoptArgumentError if the user's argv did not match the usage patterns | |||
| std::map<std::string, value> DOCOPT_API docopt_parse(std::string const& doc, | |||
| DOCOPT_API | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm a little nervous about swapping the order of these (the return type and the annotation). Was this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
__declspec(dllexport)/__declspec(dllimport) should be of left-side */& (MSVC specfic). This change for one-style with operator<<
docopt_api.h Outdated
| // docopt | ||
| // | ||
| // Created by Dmitriy Vetutnev on 2019-09-05. | ||
| // Copyright (c) 2019 Dmitriy Vetutnev. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we remove the attribution and copyright here? I don't think we necessarily needany copyright attribution outside of the top one. (In other words, I'm not asking you to change it match the docopt.h header, just leave only the lines 1-4 and delete 5 and 6).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, this lines removed
docopt_value.h Outdated
| @@ -105,7 +107,7 @@ namespace docopt { | |||
| }; | |||
| /// Write out the contents to the ostream | |||
| std::ostream& operator<<(std::ostream&,valueconst&); | |||
| DOCOPT_APIstd::ostream& operator<<(std::ostream&, const value&); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please change back tovalue const& (the style for this library is east-const)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, changed
This looks good. Can you vouch that MSVC is happy with this? I have no Windows machines myself to try on. |
Yes. I`am build on Windows 7 MSVC 2017. |
No description provided.