Fixes header includes to conform to Google standards #24
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Did you really have problems compiling on Linux? Linux has been my main development system for this since I started (with Mac as my secondary). |
|
Yes, it will not compile with g++ 5 under Ubuntu 16. I tried Ubuntu 18 and
had the same issues. Which Linux dist do you use? Are you using clang or
g++ to compile?
Fundamentally, application include guards are not supposed to begin with
underscores. This change will make things more correct, albeit at the
expense of changing a lot of files.
The next issue I am hitting is that g++ does not default to modern c++
standards so passing an extra command line arg helps with this.
Compatibility with macOS and clang is maintained by adding this option. I
can put this in a separate PR.
There may be other issues with respect to Math.h and case insensitive file
systems. This can also be a separate PR.
…On Wednesday, January 2, 2019, Michael Kohn ***@***.***> wrote:
Did you really have problems compiling on Linux? Linux has been my main
development system for this since I started (with Mac as my secondary).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#24 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKPkGpDrCG4KWWxaa3hIsGHQdt5WZ93ks5u_KcjgaJpZM4ZmIcq>
.
|
|
Thank you for the Merge!
…On Wednesday, January 2, 2019, Michael Kohn ***@***.***> wrote:
Did you really have problems compiling on Linux? Linux has been my main
development system for this since I started (with Mac as my secondary).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#24 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKPkGpDrCG4KWWxaa3hIsGHQdt5WZ93ks5u_KcjgaJpZM4ZmIcq>
.
|
|
The past couple days I was building with Ubuntu 18.04 and 18.10. I was also able to build it with FreeBSD (looks like clang 3.4.1). Thanks for this merge request... I'm going to change the include guards like this in naken_asm also tonight. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes java_grinder include guards to conform to Google standards. See https://stackoverflow.com/questions/4867559/naming-include-guards
Note that it is NOT legal to begin include guards with leading
_or__because doing so causes issues with system headers. For example, Linux'smath.hfile looks like:Because java_grinder defines
Math.hwith the identical header guard, it is not currently possible to compile java_grinder under Linux. This change brings java_grinder one step closer to compiling under Linux.