Skip to content
Marcel edited this page Dec 17, 2018 · 7 revisions

Sprint 2018/11/26 - 2018/12/17 by marcel (@marehr)

quick facts:

  • 3 open PRs (~33 comments)
  • 15 merged PRs (~161 comments, merged:2018-11-26..2018-12-17)
  • altogether ~194 comments (~11 comments per pr)
  • 11 reviews
  • 2 open reviews

3 major projects:

  • modularizing alphabet test cases (opened Oct 29th)
  • introduce code coverage test and nice overview (opened Jun 6th)
  • (re-)introduce strong warning levels (opened Dec 5th)

modularizing alphabet test cases

each concept models something

=> all types which model the concept should only include a concept ("meta") tests.

How we did this before:

  • name concept tests as alphabet_test.cpp, ...
  • use TYPED_TEST_CASE
  • add all types which model the concept in that test

Cons with this approach:

  • each added type increases build time (by a lot!)
  • hard to maintain, it's easy to forget to add a new type
  • if one type breaks, because of compiler changes, it is a mess to locate the problem
  • the name alphabet_test did not imply any model test

How we change it:

  • add a new alphabet_test_template.hpp file which contains all tests for a concept/model
  • each type includes in its own test this header
  • and instantiates it

Example, template for a concept test:

https://github.com/seqan/seqan3/blob/c7aa6a711c0b0d6a8bc8eff4a62034403eeb512c/test/unit/alphabet/alphabet_test_template.hpp#L30-L39

Example, type which includes the concept test:

https://github.com/seqan/seqan3/blob/c7aa6a711c0b0d6a8bc8eff4a62034403eeb512c/test/unit/alphabet/nucleotide/dna4_test.cpp#L8-L14

Pros:

  • reduces build time (by enabling parallel builds)
  • easy to add a new type
  • compiler problems with a type is more locally now
  • better name for model tests
  • (each header file has now it's own test cases; easier to investigate problems)

Cons:

introduce code coverage test and nice overview

Just look at https://github.com/seqan/seqan3/pull/607#issuecomment-447756161

and then at https://codecov.io/gh/seqan/seqan3

(re-)introduce strong warning levels

We have strong warning levels again.

I fixed mostly

  • unsigned != signed comparison warnings.
  • stmt1 && stmt2 || stmt3 || stmt4 && stmt5 bracket warnings (chris loves them).
  • unused parameters warnings
  • narrowing conversions warnings (int -> char)
  • uninitialized warnings

IMPORTANT

If you encounter problems with our test cases ISO C++11 requires at least one argument for the "..." in a variadic macro [-Werror].

Please checkout 52f8183e7f3620cf03f321a2624eb0d4f7649f4c in <build-folder>/vendor/googletest/.

New detail::API for_each on parameter packs / type lists / types

https://github.com/seqan/seqan3/blob/c7aa6a711c0b0d6a8bc8eff4a62034403eeb512c/test/snippet/core/algorithm/for_each_value.cpp#L1-L28

https://github.com/seqan/seqan3/blob/c7aa6a711c0b0d6a8bc8eff4a62034403eeb512c/test/snippet/core/algorithm/for_each_type.cpp#L1-L54

https://github.com/seqan/seqan3/blob/c7aa6a711c0b0d6a8bc8eff4a62034403eeb512c/test/snippet/core/algorithm/for_each_type_list.cpp#L1-L57

2 projects not finished yet.

  • simd (planned for sprint)
  • clang support (not planned, but checked once in a while)

There were some hickups and blockers:

  • simd types can't be evaluated at compile time

    • gcc can't set simd[i] = 15 within a constexpr loop
    • clang can't even access simd[i] within constexpr context
    • workarounds by constructing them at compile time didn't auto-vectorize
    • clang does not auto-vectorize at all
    • => decided to make algorithms (i.e., min, max, fill, ...) on simd types non-constexpr
  • I wanted to introduce a simd literal:

    problem:

    using uint16x8_t = simd_type_t<uint16_t, 8>;
    /*constexpr*/ uint16x8_t a = 5; // this does NOT work for every compiler

    current solution:

    using uint16x8_t = simd_type_t<uint16_t, 8>;
    /*constexpr*/ uint16x8_t a = fill<uint16x8_t>(5); // this does work for every compiler

    cool solution:

    using uint16x8_t = simd_type_t<uint16_t, 8>;
    /*constexpr*/ uint16x8_t a = 5_simd<uint16x8_t>; // this would be cool, does not work

    cool(er?) solution:

    using uint16x8_t = simd_type_t<uint16_t, 8>;
    /*constexpr*/ uint16x8_t a = 5_simd; // this would be cool, gave me the following error

    error:

    In file included from simd_literal.cpp:1:
    /seqan3/include/seqan3/core/simd/concept.hpp: In function ‘void d()’:
    /seqan3/include/seqan3/core/simd/concept.hpp:87:35: error: invalid use of incomplete type ‘class b’
         requires std::Same<decltype(a - b), simd_t>;
                                     ~~^~~
    

    I could narrow this down to:

    #include <seqan3/core/simd/concept.hpp>
    class b;
    a(b *);
    void a(...);
    struct c {
      template <seqan3::simd_concept f> operator f();
    };
    void d() {
      c e;
      a(e);
    }

    not sure how to fix this! Will report bug at gcc, clang works.

clang support

clang does work for a lot of code already. After the alphabet redesign, clang does not like the defaulted constexpr constructors anymore.

Clone this wiki locally