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

Improvements for cplusplus #1223

Merged
merged 4 commits into from Oct 9, 2017

Conversation

Projects
None yet
6 participants
@morrone
Copy link
Contributor

morrone commented Oct 6, 2017

Some improvements to make flux more friendly to linking with a C++ compiler:

  • fixed a const correctness issue that C++ enforces, but C does not
  • Added, 'extern "c"' directives to all of the flux public headers, and hacked the python binding generation script to accomodate that change.

Also some simple header cleanups found along the way.

morrone added some commits Sep 25, 2017

libflux: Fix C++ compilation warning in msg_handler.h
C++ is more strict than C about maintaining const correctness.
The C++ compiler reports this warning when using the struct
flux_msg_handler_spec with flux_msg_handler_addvev().

job-ingest.cpp:50:1: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwrite-strings]
 };

We add const to the topic_glob to avoid the warning.
libflux: Cleanup includes in headers
Move includes that are only used by the implementation
and not the interface from the .h file to the .c file.

Remove unused includes.
libflux: Remove 1 from include guard definition
Remove the definiton of _FLUX_CORE_KEEPALIVE_H to 1.  This just makes
it the same as all of the other headers in the directory.
headers: Add 'extern "C"' to headers for C++
Add 'extern "C"' linkage directive to all of the headers used
by the externally accessible flux headers: core.h and optparse.h.
It is conditionally added when __cplusplus is defined.

These headers should cover the needs of most flux modules.  The
directive can be added to additional headers as needed.

In order to accomodate the new directive, the python
"make_binding.py" script needed work.  The python CFFI (C Foreign
Function Interface) is only able to compile straight C code; it
does not have a C preprocessor, nor does it understand the
'extern "C"' linkage directive.

make_binding.py preprocesses the headers itself.  It honors
'#include ""' statements (but intentionally avoids '#include <>')
by inlining the requested file.  All other macros are simply
discarded, including conditionals.

Ignoring conditional statements means that the contents of
preprocessor conditionals are always included.  Potentional two
conflicting definitions could appear if there is an #else statement.

Code is added to make_binding.py to recognize and handle #ifdef
and #ifndef (with or without #else) only when the condition
is '__cplusplus'.

If this problem arises again we may need to figure out how to
save the real C preprocessor output of all of our headers at normal
flux compile time, and then feed that to CFFI instead of our
quick-and-dirty manual preprocessing.

Fixes #1199

@morrone morrone requested review from garlick and grondo Oct 6, 2017

@grondo

grondo approved these changes Oct 6, 2017

Copy link
Contributor

grondo left a comment

Very nice cleanup. Thanks for cleaning up extraneous #include in API headers, and for figuring out the python binding issue!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 6, 2017

Coverage Status

Coverage increased (+0.04%) to 78.657% when pulling 79d79db on morrone:improvements_for_cplusplus into 96a165c on flux-framework:master.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Oct 6, 2017

LGTM too. I don't understand why CI failed with Clang though...

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 7, 2017

Codecov Report

Merging #1223 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1223      +/-   ##
==========================================
+ Coverage   78.21%   78.25%   +0.03%     
==========================================
  Files         158      158              
  Lines       29298    29298              
==========================================
+ Hits        22916    22926      +10     
+ Misses       6382     6372      -10
Impacted Files Coverage Δ
src/common/libflux/response.c 84.55% <ø> (ø) ⬆️
src/common/libflux/event.c 90.32% <ø> (ø) ⬆️
src/common/libflux/request.c 88.46% <ø> (ø) ⬆️
src/common/libflux/rpc.c 94.21% <ø> (ø) ⬆️
src/common/libflux/mrpc.c 86.66% <ø> (+1.17%) ⬆️
src/common/libflux/msg_handler.c 86.46% <100%> (ø) ⬆️
src/common/libutil/blobref.c 97.22% <0%> (-1.39%) ⬇️
src/broker/overlay.c 72.28% <0%> (-0.36%) ⬇️
src/common/libcompat/rpc.c 94.61% <0%> (ø) ⬆️
src/common/libflux/message.c 81.25% <0%> (ø) ⬆️
... and 7 more

@morrone morrone removed the request for review from garlick Oct 8, 2017

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 9, 2017

Looks great, thanks @morrone!

@garlick garlick merged commit fd68af2 into flux-framework:master Oct 9, 2017

4 checks passed

codecov/patch 100% of diff hit (target 78.21%)
Details
codecov/project 78.25% (+0.03%) compared to 96a165c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 78.657%
Details

@morrone morrone deleted the morrone:improvements_for_cplusplus branch Oct 12, 2017

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.