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

fixes for gcc-4.7 #8

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

fixes for gcc-4.7 #8

wants to merge 7 commits into from

Conversation

dmitris
Copy link
Collaborator

@dmitris dmitris commented Feb 15, 2013

...e with gcc-4.7 - fixes issue #7

(Compiled on Fedora 17 x86-64, need to verify that it still compiles on Ubuntu/Debian)

@wisec
Copy link
Owner

wisec commented Feb 15, 2013

Thanks Dmitry,
there are some comments about conditional compilation which is missing.
The code you added will break windows compilation for sure
and unfortunately this prevents me from accepting the pull request.
On the other side, thanks a lot for your effort, another guy is working on 64bit compilation on new gcc 4.7 and most of your code confirms some of the patches we were working on.

Also I have some comment that I'll add to the code.

@dmitris
Copy link
Collaborator Author

dmitris commented Feb 15, 2013

sorry there were more changes that necessary - thanks for double-checking (I'm glad I used a PR :) ) I reverted the lines that you commented on. See if you can use it now!

I verified that after reversion it compiles on Fedora (x86_64) and on Ubuntu 12 LTS (32-bit).

@wisec
Copy link
Owner

wisec commented Feb 15, 2013

hehe you're right!
better using always pull requests just to be sure ! :)
I'll merge it as soon as I can review the code.
Thanks!

@dmitris
Copy link
Collaborator Author

dmitris commented Feb 15, 2013

I see that conditional compilation guards need to be added around #include <unistd.h> lines - what is the typical or recommended style? #ifdef linux ?

@wisec
Copy link
Owner

wisec commented Feb 15, 2013

have a look at :
https://hg.mozilla.org/mozilla-central/rev/87a5ed480992

this is one of the patches for the missing inclusion of unistd issue.

@dmitris
Copy link
Collaborator Author

dmitris commented Feb 15, 2013

OK - copied the guards from the mozilla-central file, should be good now. Sorry for bunch of commits! Compiles on Fedora but I did not try it on Windows yet.

@dmitris
Copy link
Collaborator Author

dmitris commented Feb 18, 2013

Confirmed that my repository https://github.com/dmitris/DOMinator builds fine on Windows (with MozillaBuild and pymake), tests run fine, Firefox application opens.

The PR should be now fine to merge - let me know if you see any remaining problems with it. Thanks.

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.

None yet

2 participants