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

Fix VC++ build warnings #68

Closed
wants to merge 6 commits into from
Closed

Fix VC++ build warnings #68

wants to merge 6 commits into from

Conversation

ThePhD
Copy link
Contributor

@ThePhD ThePhD commented Dec 8, 2014

This PR is a simple PR to fix errors with VC++'s compiler an warnings about conversions from integers to booleans. Otherwise, the build is errorless.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.36%) when pulling 669daee on ThePhD:master into c0e95ab on behdad:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.28%) when pulling 8bf4e44 on ThePhD:master into c0e95ab on behdad:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.69%) to 58.92% when pulling 045f003 on ThePhD:master into ca1c281 on behdad:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.58%) to 58.62% when pulling 6e01004 on ThePhD:master into 7888a6b on behdad:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.43%) to 58.47% when pulling b28e102 on ThePhD:master into 7888a6b on behdad:master.

behdad added a commit that referenced this pull request Apr 8, 2015
@behdad
Fix warnings
b931e0b 
Part of #68
@ThePhD
Copy link
Contributor Author

ThePhD commented Jun 17, 2015

Hey! I saw you fix up the warnings. I'm going to run another build, and if things are okay I hope you don't mind me closing this pull request!

@ThePhD
Copy link
Contributor Author

ThePhD commented Jun 17, 2015

So the only leftover error is C4800 for VC++, which is the hb_bool_t: forcing value to bool 'true' or 'false' (performance warning) nonsense.

The best way to fix those is to just compare against 0, I think (apparently the !! idiom doesn't make the warning go away).

Here's the build log with the affected files: https://gist.github.com/ThePhD/14c210388fb3e85cea6b .

@behdad
Copy link
Member

behdad commented Jun 18, 2015

Thanks. I'll take a look.

@ThePhD ThePhD mentioned this pull request Jun 23, 2015
Merged
@ThePhD
Copy link
Contributor Author

ThePhD commented Jun 23, 2015

I've updated the master branch on my PR. There was an additional hard error that came from harfbuzz recently:
harfbuzz\src\hb-common.cc(222): error C4996: 'strdup': The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _strdup. See online help for details.

It looks like VC++ supports _strdup, so I added it to this PR's commits. I'm fairly certain g++ and clang++ both have _strdup available...

Finally, I have to close this issue because of some branch and history fixes I had to make to work on the most up-to-date branch rather than my outdated one, so a new Pull Request was opened at #114: sorry if it's confusing!

@ThePhD ThePhD closed this Jun 23, 2015
@ThePhD ThePhD deleted the master branch August 14, 2015 05:48
gpgreen pushed a commit to gpgreen/harfbuzz that referenced this pull request Jan 10, 2024
Auto merge of harfbuzz#68 - larsbergstrom:fix_android, r=mbrubeck
9c29e1d 
Fix android

r? @mbrubeck

The issue here is that now that we're not using the NDK toolchain, the compilers won't "magically" figure out all of the include paths. These have all been set up in `CXXINCLUDE` but we need to pass them to `CPPINCLUDE` in order for the C preprocessor to also be able to pick up the hodge-podge of include directories.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-harfbuzz/68)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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
@ThePhD @coveralls @behdad