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

Define a public API #7

Merged
merged 4 commits into from Oct 10, 2017

Conversation

Projects
None yet
3 participants
@zeehio
Contributor

zeehio commented Sep 14, 2017

This commit sets the default visibility to hidden and exports all the symbols required to compile mimic-full.

Some of those symbols should be private, for instance:

  • ffeature_string should be private as there is the equivalent mimic_ffeature_string

This API should little by little be reduced to something well designed and all public functions should be prefixed with mimic_ whenever possible to prevent clashes when used with other programs.

This is needed in order to build mimic in Windows with the msvc compiler

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 14, 2017

Codecov Report

Merging #7 into development will increase coverage by <.01%.
The diff coverage is 28.57%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development       #7      +/-   ##
===============================================
+ Coverage        17.81%   17.82%   +<.01%     
===============================================
  Files               79       80       +1     
  Lines             8867     8874       +7     
===============================================
+ Hits              1580     1582       +2     
- Misses            7287     7292       +5
Impacted Files Coverage Δ
src/utils/cst_args.c 0% <ø> (ø) ⬆️
src/utils/cst_endian.c 0% <ø> (ø) ⬆️
src/cg/cst_cg_map.c 0% <ø> (ø) ⬆️
src/speech/cst_wave_io.c 18.03% <ø> (ø) ⬆️
src/audio/auserver.c 0% <ø> (ø) ⬆️
src/speech/cst_track_io.c 0% <ø> (ø) ⬆️
src/wavesynth/cst_reflpc.c 0% <ø> (ø) ⬆️
src/audio/audio.c 2.48% <ø> (ø) ⬆️
src/audio/au_alsa.c 2.77% <ø> (ø) ⬆️
src/synth/cst_ffeatures.c 0% <ø> (ø) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 639d470...d5510d0. Read the comment docs.

codecov-io commented Sep 14, 2017

Codecov Report

Merging #7 into development will increase coverage by <.01%.
The diff coverage is 28.57%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development       #7      +/-   ##
===============================================
+ Coverage        17.81%   17.82%   +<.01%     
===============================================
  Files               79       80       +1     
  Lines             8867     8874       +7     
===============================================
+ Hits              1580     1582       +2     
- Misses            7287     7292       +5
Impacted Files Coverage Δ
src/utils/cst_args.c 0% <ø> (ø) ⬆️
src/utils/cst_endian.c 0% <ø> (ø) ⬆️
src/cg/cst_cg_map.c 0% <ø> (ø) ⬆️
src/speech/cst_wave_io.c 18.03% <ø> (ø) ⬆️
src/audio/auserver.c 0% <ø> (ø) ⬆️
src/speech/cst_track_io.c 0% <ø> (ø) ⬆️
src/wavesynth/cst_reflpc.c 0% <ø> (ø) ⬆️
src/audio/audio.c 2.48% <ø> (ø) ⬆️
src/audio/au_alsa.c 2.77% <ø> (ø) ⬆️
src/synth/cst_ffeatures.c 0% <ø> (ø) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 639d470...d5510d0. Read the comment docs.

@forslund

This comment has been minimized.

Show comment
Hide comment
@forslund

forslund Sep 19, 2017

Member

We're using the -fvisibility=hidden argument so MIMIC_CORE_PRIVATE will be implicit if we leave it out. (if I understand things correct?)

I see that in two places it is used, so I guess it's necessary sometimes. Is there a good guide somewhere to tell how to determine if the hidden attribute needs to be explicit?

Member

forslund commented Sep 19, 2017

We're using the -fvisibility=hidden argument so MIMIC_CORE_PRIVATE will be implicit if we leave it out. (if I understand things correct?)

I see that in two places it is used, so I guess it's necessary sometimes. Is there a good guide somewhere to tell how to determine if the hidden attribute needs to be explicit?

@@ -37,8 +37,10 @@
/* Basic wraparounds for malloc and free */
/* */
/*************************************************************************/
#ifndef __CST_ALLOC_H__

This comment has been minimized.

@zeehio

zeehio Sep 19, 2017

Contributor

This change (and all the similar ones) are done because macros starting with underscore are reserved at least in the GNU lib C library so it is good practice to avoid them

@zeehio

zeehio Sep 19, 2017

Contributor

This change (and all the similar ones) are done because macros starting with underscore are reserved at least in the GNU lib C library so it is good practice to avoid them

@zeehio

This comment has been minimized.

Show comment
Hide comment
@zeehio

zeehio Sep 19, 2017

Contributor

This is the criteria I have tried to follow.

There is quite a bit of information to digest and I am totally open for opinions and clarification, as this is something I have made up from what I have learnt, and there may be mistakes or holes in these rules.

There are some questions and clarifications in the end. If you believe this is overly complex, feel free to say so, it is the best I could come up with to cover all mimic-core use cases.

Seven definitions:

a) A core executable is an executable compiled by this repository (e.g. mimic-core, t2p)
b) A non-core executable is any executable that is not a core executable.
c) A public header is a header that is installed, so all executables can use it.
d) A private header is a header that is not installed, so non-core executables can't even see them.
e) A public symbol is a symbol that can be used by any executable.
f) A private symbol is a symbol that is only used by the libmimiccore library.
g) A protected symbol is a symbol that is used by the libmimiccore library and core executables.

Ten rules:

  1. Each symbol is only found in one header.
  2. Public symbols should be tagged with MIMIC_CORE_PUBLIC
  3. Private symbols should be tagged with MIMIC_CORE_PRIVATE.
  4. Protected symbols have to be tagged with MIMIC_CORE_PUBLIC (they need to be exported from the library as well)
  5. Headers with at least one public symbol are public headers.
  6. Any header included by a public header is also a public header.
  7. Public headers should try to only include public symbols, not protected or private symbols.
  8. Private headers can include public headers.
  9. Private headers should have a file name like *_internal.h.
  10. Tests should be allowed to call any kind of symbol (to test it)

Some details:

  • I am ignoring rule 10 because I have not found a way to comply with it. If I need the symbol I make it protected.
  • I ignored rule number 9 in the file name of the private header "include/cst_endian.h" because I just invented the rule while writing this. 10 is a round number, so it was worth inventing the rule.
  • As you said, by default symbols are private. Both on Windows (it's the default) and on gcc/clang compilers (that have -fvisibility=hidden).
  • Why are there MIMIC_CORE_PRIVATE tags if it is the default?
    • I used the MIMIC_CORE_PRIVATE tag when I reviewed the symbol and I considered it did not make sense for mimic-core to export it. It is a kind of "hey, the symbol has been reviewed and marked as private". Other symbols without this tag are now private, and not required to compile mimic-full, but maybe are good candidates to be exported. Additionally, private symbols should be moved to private headers if possible, so I did not use the MIMIC_CORE_PRIVATE tag in a public header.
  • Do you think it would be nice to add the definition #define MIMIC_CORE_PROTECTED MIMIC_CORE_PUBLIC so it is easier for us to distinguish protected symbols from public symbols? For instance the symbols swapfloat and swapdouble in the --for now only-- private header include/cst_endian.h are protected but they have the MIMIC_CORE_PUBLIC flag.
    • I believe it would be a good choice, and if you like this maybe I would also add this post to the include/cst_lib_visibility.h header file to document this.

Thanks for any feedback! Even "it's overly complex, I don't like it" would be good!

Contributor

zeehio commented Sep 19, 2017

This is the criteria I have tried to follow.

There is quite a bit of information to digest and I am totally open for opinions and clarification, as this is something I have made up from what I have learnt, and there may be mistakes or holes in these rules.

There are some questions and clarifications in the end. If you believe this is overly complex, feel free to say so, it is the best I could come up with to cover all mimic-core use cases.

Seven definitions:

a) A core executable is an executable compiled by this repository (e.g. mimic-core, t2p)
b) A non-core executable is any executable that is not a core executable.
c) A public header is a header that is installed, so all executables can use it.
d) A private header is a header that is not installed, so non-core executables can't even see them.
e) A public symbol is a symbol that can be used by any executable.
f) A private symbol is a symbol that is only used by the libmimiccore library.
g) A protected symbol is a symbol that is used by the libmimiccore library and core executables.

Ten rules:

  1. Each symbol is only found in one header.
  2. Public symbols should be tagged with MIMIC_CORE_PUBLIC
  3. Private symbols should be tagged with MIMIC_CORE_PRIVATE.
  4. Protected symbols have to be tagged with MIMIC_CORE_PUBLIC (they need to be exported from the library as well)
  5. Headers with at least one public symbol are public headers.
  6. Any header included by a public header is also a public header.
  7. Public headers should try to only include public symbols, not protected or private symbols.
  8. Private headers can include public headers.
  9. Private headers should have a file name like *_internal.h.
  10. Tests should be allowed to call any kind of symbol (to test it)

Some details:

  • I am ignoring rule 10 because I have not found a way to comply with it. If I need the symbol I make it protected.
  • I ignored rule number 9 in the file name of the private header "include/cst_endian.h" because I just invented the rule while writing this. 10 is a round number, so it was worth inventing the rule.
  • As you said, by default symbols are private. Both on Windows (it's the default) and on gcc/clang compilers (that have -fvisibility=hidden).
  • Why are there MIMIC_CORE_PRIVATE tags if it is the default?
    • I used the MIMIC_CORE_PRIVATE tag when I reviewed the symbol and I considered it did not make sense for mimic-core to export it. It is a kind of "hey, the symbol has been reviewed and marked as private". Other symbols without this tag are now private, and not required to compile mimic-full, but maybe are good candidates to be exported. Additionally, private symbols should be moved to private headers if possible, so I did not use the MIMIC_CORE_PRIVATE tag in a public header.
  • Do you think it would be nice to add the definition #define MIMIC_CORE_PROTECTED MIMIC_CORE_PUBLIC so it is easier for us to distinguish protected symbols from public symbols? For instance the symbols swapfloat and swapdouble in the --for now only-- private header include/cst_endian.h are protected but they have the MIMIC_CORE_PUBLIC flag.
    • I believe it would be a good choice, and if you like this maybe I would also add this post to the include/cst_lib_visibility.h header file to document this.

Thanks for any feedback! Even "it's overly complex, I don't like it" would be good!

@forslund

This comment has been minimized.

Show comment
Hide comment
@forslund

forslund Sep 19, 2017

Member

I understand your reasoning here and it mostly sound sane :)

I do think a MIMIC_CORE_PROTECTED macro would be useful, otherwise I'd be "Hey why is this exposed as a public?", remove it, get failing tests and realize why it was there :)

Regarding MIMIC_CORE_PRIVATE. I guess it will be useful in the beginning at least, might be a hurdle for (possible) new contributor (/lazy swedes) to have to mark new functions. In the least we should see if we can have an automatic test to make sure the code keeps some sort of structure.

Member

forslund commented Sep 19, 2017

I understand your reasoning here and it mostly sound sane :)

I do think a MIMIC_CORE_PROTECTED macro would be useful, otherwise I'd be "Hey why is this exposed as a public?", remove it, get failing tests and realize why it was there :)

Regarding MIMIC_CORE_PRIVATE. I guess it will be useful in the beginning at least, might be a hurdle for (possible) new contributor (/lazy swedes) to have to mark new functions. In the least we should see if we can have an automatic test to make sure the code keeps some sort of structure.

zeehio added some commits Sep 14, 2017

Define a public API unstable draft
This commit sets the default visibility to hidden and exports
all the symbols required to compile mimic-full.

Some of those symbols should be private, for instance:
 - ffeature_string should be private as there is the equivalent mimic_ffeature_string

This API should little by little be reduced to something well designed
and all public functions should be prefixed with mimic_ whenever possible
to prevent clashes.
Fix header installation
The `cst_endian.h` header is not used outside `mimic-core`, so there is no need to install it.
However, the `cst_error.h` is used outside `mimic-core` and should be installed.
API Visibility fixes
- Fix errors according to latest development commits.
- cst_endian.h renamed to cst_endian_internal.h.
- Library visibility rules documented in cst_lib_visibility.h.
- Added MIMIC_CORE_PROTECTED definition for protected symbols.
- MIMIC_CORE_PROTECTED and MIMIC_CORE_PRIVATE are undefined if non-core
  executables are compiled. With this measure, the compilation of other
  programs will fail if we leave private symbols in public headers.
- If we want we can now write a test that includes all public headers and
  that it is built as a non-core executable. The test will fail if
  there are MIMIC_CORE_PROTECTED or MIMIC_CORE_PRIVATE symbols in
  any of the headers.
- Moved t2p function for language selection to the libmimiccore library.
@zeehio

This comment has been minimized.

Show comment
Hide comment
@zeehio

zeehio Sep 20, 2017

Contributor

I have rebased the pull request to be up to date with the latest commits in development. I have also changed the symbol marks in cst_endian.h from public to protected when necessary and renamed it to cst_endian_internal.h.

Message from the two last commits:

  • Fix errors according to latest development commits.

  • cst_endian.h renamed to cst_endian_internal.h.

  • Library visibility rules documented in cst_lib_visibility.h.

  • Added MIMIC_CORE_PROTECTED definition for protected symbols.

  • MIMIC_CORE_PROTECTED and MIMIC_CORE_PRIVATE are undefined if non-core
    executables are compiled. With this measure, the compilation of other
    programs will fail if we leave private symbols in public headers.

  • Moved t2p function for language selection to the libmimiccore library.

  • Added a test that includes all public headers and
    that it is built as a non-core executable. The test will fail if
    there are MIMIC_CORE_PROTECTED or MIMIC_CORE_PRIVATE symbols in
    any of the public headers.

Contributor

zeehio commented Sep 20, 2017

I have rebased the pull request to be up to date with the latest commits in development. I have also changed the symbol marks in cst_endian.h from public to protected when necessary and renamed it to cst_endian_internal.h.

Message from the two last commits:

  • Fix errors according to latest development commits.

  • cst_endian.h renamed to cst_endian_internal.h.

  • Library visibility rules documented in cst_lib_visibility.h.

  • Added MIMIC_CORE_PROTECTED definition for protected symbols.

  • MIMIC_CORE_PROTECTED and MIMIC_CORE_PRIVATE are undefined if non-core
    executables are compiled. With this measure, the compilation of other
    programs will fail if we leave private symbols in public headers.

  • Moved t2p function for language selection to the libmimiccore library.

  • Added a test that includes all public headers and
    that it is built as a non-core executable. The test will fail if
    there are MIMIC_CORE_PROTECTED or MIMIC_CORE_PRIVATE symbols in
    any of the public headers.

@forslund forslund merged commit a2b9655 into development Oct 10, 2017

3 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@zeehio zeehio deleted the api_visibility branch Oct 10, 2017

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