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

ml_utils.c: change use of c++11 posix standard function "gethostname"… #1810

Closed

Conversation

prwolfe
Copy link
Contributor

@prwolfe prwolfe commented Oct 3, 2017

… to c99 standard "uname"

Since cmake interprets this as a c file by name it
only pulls in posix functions from the 98 standard.
That meant that "gethostname" was an implicit declaration
and that fails when running with warnings-as-errors.

… to c99 standard "uname"

Since cmake interprets this as a c file by name it
only pulls in posix functions from the 98 standard.
That meant that "gethostname" was an implicit declaration
and that fails when running with warnings-as-errors.
@prwolfe prwolfe self-assigned this Oct 3, 2017
@bartlettroscoe bartlettroscoe added the stage: in progress Work on the issue has started label Oct 3, 2017
@prwolfe prwolfe requested a review from ibaned October 3, 2017 18:58
@aprokop
Copy link
Contributor

aprokop commented Oct 3, 2017

@trilinos/ml

@aprokop aprokop added the pkg: ML label Oct 3, 2017
@aprokop aprokop requested a review from jhux2 October 3, 2017 19:26
@ibaned
Copy link
Contributor

ibaned commented Oct 3, 2017

I don't think these functions have anything to do with the C++11 and C99 standards at all? They are both in the POSIX.1-2001 standard. POSIX is only a C API standard, has nothing to do with C++, and as far as I know also doesn't depend on the version of the C standard being used. Can you post the configuration you were using and the warning that was emitted?

@prwolfe
Copy link
Contributor Author

prwolfe commented Oct 4, 2017

Sure - this comes directly from the dashboard in the sierra replication builds. See https://testing.sandia.gov/cdash/viewBuildError.php?buildid=3143344

I did look into the unistd.h header and the notes from the gcc compiler team are what I was referencing as to the versions. I did confirm that the macros in units.h turn off declaration of gethostname when using -std=c99 which this does. I also tried to use -std=c++11 and the compiler said no as it's c code. We could work through and get it to use -std=c11 (we currently don't pass anything - not sure where c99 came from.)

@prwolfe
Copy link
Contributor Author

prwolfe commented Oct 4, 2017

It looks like the compiler default will be c11 with gnu extensions. I think I will talk with Jim and Mike to see if all of Trilinos should be using that or the stricter c11. Sierra does not pass anything and so would get the gnu extensions.

Thanks!

@ibaned
Copy link
Contributor

ibaned commented Oct 6, 2017

Hmm you're right that its not being declared properly, I just confirmed while building Trilinos:

/home/daibane/src/Trilinos/packages/ml/src/Utils/ml_utils.c: In function ‘ML_BreakForDebugger’:
/home/daibane/src/Trilinos/packages/ml/src/Utils/ml_utils.c:1965:9: warning: implicit declaration of function ‘gethostname’ [-Wimplicit-function-declaration]
         gethostname(hostname, sizeof(hostname));
         ^~~~~~~~~~~

@prwolfe
Copy link
Contributor Author

prwolfe commented Oct 26, 2017

@jwillenbring - did this ever get discussed?

@mhoemmen
Copy link
Contributor

@prwolfe Not that I recall, though I'm not @jwillenbring :-) . I would say, if it works, merge it.

@prwolfe
Copy link
Contributor Author

prwolfe commented Nov 8, 2017

Pull request #1974 will take care of this until a full policy is established.

@prwolfe
Copy link
Contributor Author

prwolfe commented Nov 13, 2017

Closed and moved to issue #1985

@prwolfe prwolfe closed this Nov 13, 2017
@bartlettroscoe bartlettroscoe removed the stage: in progress Work on the issue has started label Nov 13, 2017
@prwolfe prwolfe deleted the correct_ml_utils_gethostname branch November 21, 2017 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants