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

Minor modifications. #93

Closed
wants to merge 7 commits into from
Closed

Minor modifications. #93

wants to merge 7 commits into from

Conversation

@mmaker
Copy link

mmaker commented Mar 13, 2016

Picky aesthetics for picky libraries.

@mmaker mmaker force-pushed the mmaker:master branch from 21d4761 to 159d297 Mar 13, 2016
@@ -22,8 +22,7 @@
* THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

#ifndef BITFN_H
#define BITFN_H
#pragma once

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Mar 14, 2016

This is not standard C.

This comment has been minimized.

Copy link
@mmaker

mmaker Mar 14, 2016

Author

It is standard c99 actually.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Mar 14, 2016

C99 defines #pragma directives. But not #pragma once.

This comment has been minimized.

Copy link
@mmaker

mmaker Mar 14, 2016

Author

and #pragma once is not a #pragma directive?

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Mar 14, 2016

Are you trying to lose my time ?

This comment has been minimized.

Copy link
@cfcs

cfcs Mar 14, 2016

@infinity0: I believe the problem detailed above to be an obvious deficit in Solaris Studio C/C++, and believe we should labor to have this much-needed feature added to Solaris Studio C/C++, after which we can merge this PR, since I agree - with all of my heart - with your sentiment quoted below:

especially since the world at present has a lack of well-written security code

  • and I would like to add that this is especially needed in the Solaris ecosystems of the world where software using nocrypto will most likely contribute extensively towards this goal by being the only secure code around.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Mar 14, 2016

Do we really need so much OT comments and github reactions to try to make programmers write C standard compliant code ? This place is turning into the worst kind of pointless forum.

This comment has been minimized.

Copy link
@cfcs

cfcs Mar 14, 2016

To hell with C standards, the long-term goal should be to eventually put that programming language in the grave.

On a more serious note, replacing whitespace in imported code makes it more cumbersome to diff any real changes, and I think that's a pretty compelling argument for rejecting mmaker@159d297

As far as mmaker@f02b695 and mmaker@84d6a86 go, one way to satisfy all parties would be to add a section to the Makefile that detects the platform and uses sed to replace the pragmas with if-guards in the unfortunate cases when someone is trying to use that. Either that, or recommend cross-compiling.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Mar 14, 2016

To hell with C standards, the long-term goal should be to eventually put that programming language in the grave.

That would make a very good tweet. But meanwhile the language is still here and living and just keep in mind that the list of compilers linked earlier are not the only tools that consume C code.

add a section to the Makefile that detects the platform and uses sed to replace the pragmas with if-guards in the unfortunate cases when someone is trying to use that. Either that, or recommend cross-compiling.

Is that still in the "more serious note" ?

This comment has been minimized.

Copy link
@cfcs

cfcs Mar 14, 2016

Yes, but that doesn't mean we have to specifically target broken tools that are incompatible with the majority of C compilers (with the notable exception of Sun/Oracle Solaris Studio C/C++).

But to make you happy, I found a better solution than the proposed sed script while retaining compatibility with the Solaris tools. See mmaker@6d17130

@mmaker
Copy link
Author

mmaker commented Mar 14, 2016

Speaking of standard c99™, turning on -pedantic I get:

In file included from xen/native/hash/md5.c:25:0:
/home/maker/.opam/4.02.3/lib/pkgconfig/../../include/mirage-xen-posix/include/string.h:4:2: error: #include_next is a GCC extension [-Werror]
 #include_next <string.h>
  ^
cc1: all warnings being treated as errors
Command exited with code 2.

Is this an issue then?

mmaker and others added 2 commits Mar 13, 2016
…ith Sun Solaris Studio C/C++ and other tools that consume C code.

This commit utilizes `#pragma hdrstop` of Oracle Solaris Studio C to avoid the need for ifdef-guards in the header files:
https://docs.oracle.com/cd/E24457_01/html/E21990/bjaby.html#OSSCGbjacg

According to Wikipedia, all other compilers in the world support `#pragma once`.
@cfcs cfcs force-pushed the mmaker:master branch from 84d6a86 to bd365fb Mar 14, 2016
@cfcs cfcs force-pushed the mmaker:master branch from 94fcaed to bd365fb Mar 14, 2016
@mmaker
Copy link
Author

mmaker commented Mar 14, 2016

I privately received this very elegant solution from @cfcs to fix my initial loss of support for Oracle Studio; I'm adding it to this pull request because I believe it deserves at least a quick look on your side, @dbuenzli.
@pqwy too perhaps, what do you think?

@dbuenzli
Copy link

dbuenzli commented Mar 14, 2016

I privately received this very elegant solution from @cfcs to fix my initial loss of support for Oracle Studio; I'm adding it to this pull request because I believe it deserves at least a quick look on your side, @dbuenzli.

Very elegant, you turned portable, uniform, platform independent code into untested platform dependent brittle #ifdef gibberish. Well anyways... I'm out of this discussion, it's not worth of my time.

@pqwy pqwy force-pushed the mirleft:master branch 9 times, most recently from 65e9f6c to b566a98 Mar 18, 2016
@pqwy pqwy force-pushed the mirleft:master branch 4 times, most recently from f1ec674 to 0f06ad1 Oct 27, 2016
@pqwy pqwy force-pushed the mirleft:master branch 3 times, most recently from 94940d6 to 6d0ca0e Nov 4, 2016
@pqwy pqwy force-pushed the mirleft:master branch 2 times, most recently from 4fc959e to a2ec58a Nov 4, 2016
@pqwy pqwy force-pushed the mirleft:master branch from fe12f4b to a89c616 Feb 1, 2017
@cfcs
Copy link

cfcs commented Sep 27, 2017

So... I don't think this will get merged; there's a significant backlog of other issues and PRs that could probably be considered more serious. But it does take up space in the Github view of "issues you participate in". I know this PR is important to you, but would you consider closing it, @mmaker?

PS: You missed a #ifdef guard here: https://github.com/mirleft/ocaml-nocrypto/pull/93/files#diff-62954c7dff16f3dc7aadb51c17c09cd7R24

@mmaker mmaker closed this Sep 27, 2017
@infinity0
Copy link

infinity0 commented Sep 27, 2017

But Oracle Solaris Studio now supports #pragma once!

@cfcs
Copy link

cfcs commented Sep 27, 2017

Ah and they have a much more elaborate #ifdef snippet that will lets us support practically anything!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.