Skip to content

Conversation

@marcelwa
Copy link
Contributor

@marcelwa marcelwa commented Apr 7, 2021

Libraries declared as SYSTEM do not propagate compiler warnings to top-level projects. Thereby, when using mockturtle as a library while having a stricter compiler warnings setup, this suppresses warnings originating in mockturtle. Since the test cases now link PUBLICly, you are still able to see all warnings as before while working on mockturtle. The changes only affect people including it.

For more information see, e.g., https://foonathan.net/2018/10/cmake-warnings/

A nice compiler warnings setup can be found in Jason Turner's CPP Starter Project here: https://github.com/lefticus/cpp_starter_project/blob/master/cmake/CompilerWarnings.cmake

@codecov-io
Copy link

Codecov Report

Merging #437 (7b1c12f) into master (7d0f355) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #437      +/-   ##
==========================================
- Coverage   79.74%   79.72%   -0.03%     
==========================================
  Files         117      117              
  Lines       12720    12720              
==========================================
- Hits        10144    10141       -3     
- Misses       2576     2579       +3     
Impacted Files Coverage Δ
...nclude/mockturtle/algorithms/dsd_decomposition.hpp 87.14% <0.00%> (-4.29%) ⬇️

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 7d0f355...7b1c12f. Read the comment docs.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 726086384

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 80.086%

Totals Coverage Status
Change from base Build 725561612: 0.0%
Covered Lines: 10661
Relevant Lines: 13312

💛 - Coveralls

@hriener
Copy link
Member

hriener commented Apr 7, 2021

I am wondering what prevents you from declaring mockturtle as a SYSTEM library on your side, e.g., as in #432. Do you directly include the CMakeLists.txt of mockturtle in your build process?

@marcelwa
Copy link
Contributor Author

marcelwa commented Apr 7, 2021

I am by no means an expert in CMake, so I just used add_subdirectory(mockturtle). Issue #432 refers to autotools and does not really help me, unfortunately. If you can point me towards a client-side solution, I'd be more than happy to give it a try.

@hriener
Copy link
Member

hriener commented Apr 7, 2021

Instead of adding the subdirectory, you could just add your own target for mockturtle and declare it as SYSTEM.

However, your request sounds reasonable. I would suggest adding PUBLIC to mockturtle's examples and experiments too because they are often used as drivers during development.

@marcelwa
Copy link
Contributor Author

marcelwa commented Apr 7, 2021

If I'm not mistaken, they link PUBLICly already.

Examples:

target_link_libraries(${basename} PUBLIC mockturtle)

Experiments:

target_link_libraries(${basename} PUBLIC experiments mockturtle)

@hriener hriener merged commit be3c547 into lsils:master Apr 8, 2021
@hriener
Copy link
Member

hriener commented Apr 8, 2021

Perfect! Thank you.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants