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.

Sign up for GitHub

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

Jump to bottom

Allow compiling and using as DLL in windows #502

Merged
merged 3 commits into from
May 5, 2017
Merged

Allow compiling and using as DLL in windows #502

merged 3 commits into from
May 5, 2017

Conversation

AndreasSchoenle
Copy link
Contributor

Removes warnings and errors when compiling in VS 2015 with BUILD_SHARED_LIBS set to "ON".

vitaut
vitaut requested changes Apr 29, 2017
View reviewed changes
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR! Mostly looks good, just one minor comment.

fmt/printf.h Outdated
spec.type_ = internal::DefaultType().visit(arg);

// Avoid warning for wchar_t
spec.type_ = char(internal::DefaultType().visit(arg));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change the return values in DefaultType instead of using a cast here?

@AndreasSchoenle
Copy link
Contributor Author

Changed the return type of DefaultType to 'char' as requested. Also I needed to change the assertion in void Buffer<T>::append(const U *begin, const U *end) as utiltest did not succeed in the DLL version in DEBUG builds.

The reason was that internal::to_unsigned failed when requesting a buffer of size std::numeric_limits<size_t>::max() in the FormatSystemError test as int(end-begin) is then converted to -1. In the static lib version the assertion was caught by the exception handler in format_system_error, producing the expected behavior also in Debug builds. (In Release builds the memory allocation fails, producing the same result). In the DLL version this was no longer the case.

Hopefully I did not misinterpret the intention of the test.

@AndreasSchoenle
Copy link
Contributor Author

Maybe it is possible to add appveyor tests with BUILD_SHARED_LIBS=ON?

@foonathan foonathan mentioned this pull request May 5, 2017
Merged
@foonathan foonathan merged commit 79f11db into fmtlib:master May 5, 2017
@foonathan
Copy link
Contributor

@AndreasSchoenle What do you mean by BUILD_SHARED_LIBS=ON? Could you do a PR?

@foonathan foonathan mentioned this pull request May 5, 2017
Closed
@AndreasSchoenle AndreasSchoenle deleted the fmt-api-printf-fix branch May 5, 2017 14:56
bjoernthiel pushed a commit to AbberiorInstruments/fmt that referenced this pull request May 11, 2017
@AndreasSchoenle
Allow compiling and using as DLL in windows (fmtlib#502)
0539746 
* printf.h fixed to compile clean - need to check whether this is the right
thing to do

* fix warnings and errors in test compiles with BUILD_SHARED_LIBS

* did requested changes and added one change to allow all tests to succeed
in windows DLL
foonathan pushed a commit that referenced this pull request Jun 9, 2017
@AndreasSchoenle @foonathan
Allow compiling and using as DLL in windows (#502)
6db5fe0 
* fix warnings and errors in test compiles with BUILD_SHARED_LIBS

* did requested changes and added one change to allow all tests to succeed
in windows DLL

(cherry picked from commit 79f11db)
foonathan pushed a commit that referenced this pull request Jun 9, 2017
@AndreasSchoenle @foonathan
Allow compiling and using as DLL in windows (#502)
5bce2c0 
* fix warnings and errors in test compiles with BUILD_SHARED_LIBS

* did requested changes and added one change to allow all tests to succeed
in windows DLL

(cherry picked from commit 79f11db)
vitaut added a commit that referenced this pull request Feb 2, 2018
@vitaut
Allow compiling and using as DLL in windows #502
08dff37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitaut vitaut vitaut requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AndreasSchoenle @foonathan @vitaut