[INFRA] make every seqan3 header self-contained; no exceptions anymore #1085

marehr · 2019-06-10T14:49:15Z

No description provided.

rrahn

in general looks good! Some minor comments though

rrahn · 2019-06-11T08:39:30Z

test/header/CMakeLists.txt

@@ -5,7 +5,7 @@
 # shipped with this file and also available at: https://github.com/seqan/seqan3/blob/master/LICENSE.md
 # -----------------------------------------------------------------------------------------------------

-cmake_minimum_required (VERSION 3.2)
+cmake_minimum_required (VERSION 3.6)


Have you checked that this version is compliant with all platforms that also support at least gcc-7?

I upped the version, which we already implicitly required. Because we already use the following feature which is available since 3.6

list (FILTER header_files EXCLUDE REGEX "${exclude_regex}")

Since all builds go through since forever, this should have no impact

did you update the documentation?

No, I didn't update the documentation. Or, do we want to set the minimal required version of cmake to 3.6? I'm happy to do that, but that would be out the scope of the PR.

I added a card for the dev meeting to discuss a general increase of cmake version.

See core-developers chat.

I moved the fix of the cmake versions to PR #1094 , so we can discuss this over there

rrahn · 2019-06-11T08:44:14Z

test/header/CMakeLists.txt

@@ -19,11 +19,11 @@ option (SEQAN3_FULL_HEADER_TEST "Test seqan3 headers as well as the headers of e
 # header guards are working by including the same header twice.
 #
 # example invocation:
-#     seqan3_header_test (seqan3 "<path>/include/seqan3")
+#     seqan3_header_test (seqan3 "<path>/include/seqan3" "/exclude_regex|more_exclude/")


I don't understand the example.
Should more_exclude mean I can add more exclude patterns?
Then I suggest:

Suggested change

# seqan3_header_test (seqan3 "<path>/include/seqan3" "/exclude_regex|more_exclude/")

# seqan3_header_test (seqan3 "<path>/include/seqan3" "") - Nothing will be excluded

# seqan3_header_test (seqan3 "<path>/include/seqan3" "/exclude_regex/[|more_excludes...]") - separate multiple exludes with |

or can I only specify one more exlude?

no you can give any regex. I'll change the example

I added more info, can you recheck?

smehringer · 2019-06-11T14:49:42Z

include/seqan3/std/charconv_detail.hpp

+// structs: to_chars_result, from_chars_result and chars_format
+// -----------------------------------------------------------------------------
+
+namespace std


Now non-detail stuff is in a detail folder o.O
I deliberately separated the "public API stuff I copied" from the "detail stuff to make public API work that I messed with"

That's mainly why I assigned you.

The problem is if we don't do it this way, we get weird include order issues. Because, charconv_detail.hpp needs stuff from charconv.hpp, but on the other hand charconv.hpp needs charconv_detail.hpp.

If you have an idea how to keep the current structure with the separation of the public and private API, but without this dependency loop. I'm happy to change this. The documentation will still be public even if it defined within the detail file.

Where do we get these weird issues? I thought the include order is clearly defined

// some code that is needed in detail.hpp #include <detail.hpp> // some code that is dependent on detail.hpp

I thought this is the way includes work? The only thing is that detail.hpp cannot be independently included somewhere else, but it should not need to be as it is a detail header?

You are right, but like the title says no exceptions anymore, even our detail code should be includeable in any order.

:( but why? when did we decide on that?

We resolved this today at a quick strategy meeting, right?

Yes by merging the detail header into the non-detail header, right 🤔

smehringer · 2019-06-11T14:50:29Z

test/header/CMakeLists.txt

@@ -5,7 +5,7 @@
 # shipped with this file and also available at: https://github.com/seqan/seqan3/blob/master/LICENSE.md
 # -----------------------------------------------------------------------------------------------------

-cmake_minimum_required (VERSION 3.2)
+cmake_minimum_required (VERSION 3.6)


did you update the documentation?

codecov · 2019-06-14T00:51:22Z

Codecov Report

Merging #1085 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1085   +/-   ##
=======================================
  Coverage   96.44%   96.44%           
=======================================
  Files         196      196           
  Lines        7733     7733           
=======================================
  Hits         7458     7458           
  Misses        275      275

Impacted Files	Coverage Δ
...de/seqan3/argument_parser/detail/version_check.hpp	`83.33% <ø> (ø)`	⬆️
.../seqan3/search/algorithm/detail/search_trivial.hpp	`86.11% <ø> (ø)`	⬆️
.../seqan3/search/fm_index/detail/fm_index_cursor.hpp	`100% <ø> (ø)`	⬆️
...3/search/fm_index/detail/csa_alphabet_strategy.hpp	`97.77% <ø> (ø)`	⬆️
include/seqan3/alphabet/detail/hash.hpp	`100% <ø> (ø)`	⬆️
...rch/algorithm/detail/search_scheme_precomputed.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/io/detail/misc_output.hpp	`93.33% <ø> (ø)`	⬆️

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 a6e5110...1289b88. Read the comment docs.

marehr · 2019-06-19T21:13:26Z

@rrahn can you have a second look and if everything is fine merge?

marehr requested a review from smehringer June 10, 2019 14:49

rrahn approved these changes Jun 11, 2019

View reviewed changes

smehringer reviewed Jun 11, 2019

View reviewed changes

marehr force-pushed the infra/header_test branch from 82f8c02 to dc719bc Compare June 14, 2019 00:51

marehr force-pushed the infra/header_test branch 3 times, most recently from c2a3d25 to cf789a6 Compare June 14, 2019 09:12

[INFRA] make every seqan3 header self-contained; no exceptions anymore

1289b88

marehr force-pushed the infra/header_test branch from cf789a6 to 1289b88 Compare June 17, 2019 23:58

marehr requested a review from smehringer June 17, 2019 23:59

smehringer approved these changes Jun 19, 2019

View reviewed changes

marehr requested a review from h-2 June 21, 2019 15:17

h-2 approved these changes Jun 24, 2019

View reviewed changes

h-2 merged commit a9fdbfb into seqan:master Jun 24, 2019

marehr deleted the infra/header_test branch June 24, 2019 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[INFRA] make every seqan3 header self-contained; no exceptions anymore #1085

[INFRA] make every seqan3 header self-contained; no exceptions anymore #1085

marehr commented Jun 10, 2019

rrahn left a comment •

edited

Loading

rrahn Jun 11, 2019

marehr Jun 11, 2019

smehringer Jun 11, 2019

marehr Jun 12, 2019

marehr Jun 14, 2019

marehr Jun 14, 2019

rrahn Jun 11, 2019

marehr Jun 11, 2019 •

edited

Loading

marehr Jun 14, 2019

smehringer Jun 11, 2019

marehr Jun 12, 2019

smehringer Jun 12, 2019

marehr Jun 12, 2019

smehringer Jun 13, 2019

marehr Jun 14, 2019

rrahn Jun 14, 2019

marehr Jun 17, 2019

smehringer Jun 11, 2019

codecov bot commented Jun 14, 2019 •

edited

Loading

marehr commented Jun 19, 2019

	# seqan3_header_test (seqan3 "<path>/include/seqan3" "/exclude_regex\|more_exclude/")
	# seqan3_header_test (seqan3 "<path>/include/seqan3" "") - Nothing will be excluded
	# seqan3_header_test (seqan3 "<path>/include/seqan3" "/exclude_regex/[\|more_excludes...]") - separate multiple exludes with \|

[INFRA] make every seqan3 header self-contained; no exceptions anymore #1085

[INFRA] make every seqan3 header self-contained; no exceptions anymore #1085

Conversation

marehr commented Jun 10, 2019

rrahn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marehr Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 14, 2019 • edited Loading

Codecov Report

marehr commented Jun 19, 2019

rrahn left a comment •

edited

Loading

marehr Jun 11, 2019 •

edited

Loading

codecov bot commented Jun 14, 2019 •

edited

Loading