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

DLL-inteface for operator<< (MSVC compatibility) #114

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

dvetutnev
Copy link

No description provided.

@dvetutnev dvetutnev changed the title DLL-inteface for operator<< (MSVC compotibility) DLL-inteface for operator<< (MSVC compatibility) Sep 5, 2019
Copy link
Member

@jaredgrubb jaredgrubb left a comment

Choose a reason for hiding this comment

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

In general, I prefer functional changes to be split into a separate commit from formatting or style changes.

docopt.cpp Outdated
@@ -21,10 +21,10 @@
#include <cassert>
#include <cstddef>

using namespace docopt;
namespace docopt {
Copy link
Member

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).

Copy link
Author

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
Copy link
Member

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?

Copy link
Author

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.
Copy link
Member

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 need any 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).

Copy link
Author

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&, value const&);
DOCOPT_API std::ostream& operator<<(std::ostream&, const value&);
Copy link
Member

Choose a reason for hiding this comment

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

Please change back to value const& (the style for this library is east-const)

Copy link
Author

@dvetutnev dvetutnev Sep 5, 2019

Choose a reason for hiding this comment

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

Ok, changed

@jaredgrubb
Copy link
Member

This looks good. Can you vouch that MSVC is happy with this? I have no Windows machines myself to try on.

@dvetutnev
Copy link
Author

Yes. I`am build on Windows 7 MSVC 2017.

@dvetutnev
Copy link
Author

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