[FIX] Add data access for containers #1208

eseiler · 2019-08-12T13:08:56Z

Adds raw_data() for bitcompressed_vector and allows access of the underlying SDSL vector.
Renames data() to raw_data() in concatenated_sequences since data() implies contiguous memory locations.
small_vector and small_string have data() and are contiguous, ergo no changes here.

codecov · 2019-08-14T17:19:43Z

Codecov Report

Merging #1208 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1208   +/-   ##
=======================================
  Coverage   96.84%   96.84%           
=======================================
  Files         217      217           
  Lines        8616     8616           
=======================================
  Hits         8344     8344           
  Misses        272      272

Impacted Files	Coverage Δ
.../seqan3/range/container/concatenated_sequences.hpp	`96.85% <100%> (ø)`	⬆️
...de/seqan3/range/container/bitcompressed_vector.hpp	`100% <100%> (ø)`	⬆️

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 e5fa215...b0b8ec0. Read the comment docs.

svnbgnk

The operators compare all the bitcompressed data from one vector to another, right?

seqan3/include/seqan3/range/container/bitcompressed_vector.hpp

Lines 978 to 981 in 25d06a0

    
           constexpr bool operator==(bitcompressed_vector const & rhs) const noexcept 
        
           { 
        
               return raw_data() == rhs.raw_data(); 
        
           }

svnbgnk

One thing I noticed that did not data with raw_data() swap:

seqan3/include/seqan3/range/container/bitcompressed_vector.hpp

Lines 937 to 941 in 25d06a0

    
           constexpr void swap(bitcompressed_vector & rhs) noexcept 
        
           { 
        
               std::swap(data, rhs.data); 
        
           }

.
Is there any reason for that?

test/unit/range/container/container_of_container_test.cpp

eseiler · 2019-08-20T12:56:23Z

The operators compare all the bitcompressed data from one vector to another, right?

seqan3/include/seqan3/range/container/bitcompressed_vector.hpp

Lines 978 to 981 in 25d06a0

constexpr bool operator==(bitcompressed_vector const & rhs) const noexcept

{

return raw_data() == rhs.raw_data();

}

We delegate to the compairson operator of the underlying data type (sdsl::int_vector), which then compares the content.

eseiler · 2019-08-20T12:59:32Z

One thing I noticed that did not data with raw_data() swap:

seqan3/include/seqan3/range/container/bitcompressed_vector.hpp

Lines 937 to 941 in 25d06a0

constexpr void swap(bitcompressed_vector & rhs) noexcept

{

std::swap(data, rhs.data);

}

.
Is there any reason for that?

No reason, just didn't see it. I can access the private data member within the class, but I'll change it for consistency.

svnbgnk

👍 Ok everything is resolved.

h-2

Just some minor stuff

h-2 · 2019-08-22T16:59:03Z

CHANGELOG.md

@@ -40,6 +40,10 @@ If possible, provide tooling that performs the changes, e.g. a shell-script.
 * **The `type_list` header has moved:**
  If you included `<seqan3/core/type_list.hpp>` you need to change the path to `<seqan3/core/type_list/type_list.hpp>`.

+### The `data()` function of `seqan3::concatenated_sequences` has been deprecated
+
+Use `raw_data()` instead.


The changelog format is now different...

If we mark the functions as noapi we don't need to include it in the changelog?

h-2 · 2019-08-22T17:00:00Z

include/seqan3/range/container/bitcompressed_vector.hpp

@@ -518,6 +518,19 @@ class bitcompressed_vector
        return (*this)[size()-1];
    }

+    //!\brief Direct access to the underlying SDSL vector.


Maybe you should describe more clearly what the type will be as this is exposed to the API. Note that if we expose this to the API, we cannot change the internal type anymore without breakage.

I changed it a bit. I'd like to link to a documentation of the bitvector. But there are no online docs for the sdsl3 bitvector. And the sdsl2 one looks like this.

Anyway, I noticed that in concatenated_sequences we added this sentence:
This exact representation of the data is implementation defined. Do not rely on it for API stability.
So it's basically not API and we could add \noapi for doxygen to this function?

Then we could discuss if all our container data/raw_data functions are no api.

So it's basically not API and we could add \noapi for doxygen to this function?

👍

Then we could discuss if all our container data/raw_data functions are no api.

👍

h-2 · 2019-08-22T17:00:35Z

include/seqan3/range/container/bitcompressed_vector.hpp

@@ -964,37 +977,37 @@ class bitcompressed_vector
    //!\brief Checks whether `*this` is equal to `rhs`.
    constexpr bool operator==(bitcompressed_vector const & rhs) const noexcept
    {
-        return data == rhs.data;
+        return raw_data() == rhs.raw_data();


Are these changes necessary? I don't care much ....

Mhm, I was afraid of the code coverage since I didn't want to test the data/raw_data functions.

I'll revert it and see what happens.

h-2 · 2019-08-22T17:00:57Z

include/seqan3/range/container/concatenated_sequences.hpp

+    SEQAN3_DEPRECATED_310 std::pair<decltype(data_values) const &, decltype(data_delimiters) const &> data() const
+    {
+        return raw_data();
+    }


eseiler requested review from joergi-w and svnbgnk and removed request for joergi-w August 12, 2019 13:10

eseiler force-pushed the fix/container_data branch from 7ce3123 to 25d06a0 Compare August 14, 2019 17:19

svnbgnk reviewed Aug 20, 2019

View reviewed changes

test/unit/range/container/container_of_container_test.cpp Show resolved Hide resolved

eseiler force-pushed the fix/container_data branch from 25d06a0 to f5d55a3 Compare August 20, 2019 13:04

eseiler requested a review from svnbgnk August 20, 2019 13:04

svnbgnk approved these changes Aug 21, 2019

View reviewed changes

eseiler requested a review from h-2 August 21, 2019 09:32

h-2 reviewed Aug 22, 2019

View reviewed changes

eseiler force-pushed the fix/container_data branch from f5d55a3 to 532609f Compare August 23, 2019 08:45

[FIX] Add data access for containers

b0b8ec0

eseiler force-pushed the fix/container_data branch from 532609f to b0b8ec0 Compare August 23, 2019 10:37

eseiler requested a review from h-2 August 23, 2019 12:01

h-2 approved these changes Aug 26, 2019

View reviewed changes

h-2 merged commit f2cfc5b into seqan:master Aug 26, 2019

eseiler deleted the fix/container_data branch August 26, 2019 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Add data access for containers #1208

[FIX] Add data access for containers #1208

eseiler commented Aug 12, 2019 •

edited

Loading

codecov bot commented Aug 14, 2019 •

edited

Loading

svnbgnk left a comment •

edited

Loading

svnbgnk left a comment •

edited

Loading

eseiler commented Aug 20, 2019

eseiler commented Aug 20, 2019

svnbgnk left a comment •

edited

Loading

h-2 left a comment

h-2 Aug 22, 2019

eseiler Aug 23, 2019

h-2 Aug 22, 2019

eseiler Aug 23, 2019

h-2 Aug 23, 2019

h-2 Aug 22, 2019

eseiler Aug 23, 2019

h-2 Aug 22, 2019

	constexpr bool operator==(bitcompressed_vector const & rhs) const noexcept
	{
	return raw_data() == rhs.raw_data();
	}

	constexpr void swap(bitcompressed_vector & rhs) noexcept
	{
	std::swap(data, rhs.data);
	}

[FIX] Add data access for containers #1208

[FIX] Add data access for containers #1208

Conversation

eseiler commented Aug 12, 2019 • edited Loading

codecov bot commented Aug 14, 2019 • edited Loading

Codecov Report

svnbgnk left a comment • edited Loading

Choose a reason for hiding this comment

svnbgnk left a comment • edited Loading

Choose a reason for hiding this comment

eseiler commented Aug 20, 2019

eseiler commented Aug 20, 2019

svnbgnk left a comment • edited Loading

Choose a reason for hiding this comment

h-2 left a 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

eseiler commented Aug 12, 2019 •

edited

Loading

codecov bot commented Aug 14, 2019 •

edited

Loading

svnbgnk left a comment •

edited

Loading

svnbgnk left a comment •

edited

Loading

svnbgnk left a comment •

edited

Loading