Skip to content
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

Derive iterators from std::iterator #408

Merged
merged 1 commit into from
May 24, 2020

Conversation

mikewolfram
Copy link
Contributor

...to make the containers work with algorithms from STL.
Currently modm containers won't work with algorithms like std::remove_if(). This fix resolves it.

@@ -189,7 +190,7 @@ namespace modm
*
* \todo check if a simpler implementation is possible
*/
class const_iterator
class const_iterator : public std::iterator<std::bidirectional_iterator_tag, T>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::iterator has been deprecated in C++17. See this for more information. Please use the recommended method from the article with type aliases in the iterator class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't it's deprecated. I'm building in C++17 and didn't get any warning...

@@ -258,7 +259,7 @@ namespace modm
/**
* \brief Forward iterator
*/
class iterator
class iterator : public std::iterator<std::forward_iterator_tag, T>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make sense to have a random access iterator here, but that would require extending the iterator class with operators [], +, -, +=, -=. Otherwise things like std::sort don't work which need random access.

These container implementations were originally written for xpcc before we had a standard library on all platforms and when there was no C++11. Now modm uses the standard library which provides vector, list and forward_list. I don't see why someone should prefer the modm equivalents for these three cases. I'm not sure it's worth the effort to fix them.
@salkinium We should consider to deprecate or remove the containers the standard library already provides.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would strongly prefer either using the std libc++ and/or integrating a third party one like ETL. Definitely not interested in fixing/maintaining our containers, that's something that other libs can do way better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason for using these containers instead of the standard ones, @mikewolfram? Avoiding heap? Size/efficiency concerns? What do these containers do better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, since I expected those containers to be used in modm, I wanted to use them as well for keeping it simple.

I've been already looking into ETL and was thinking about using some of it.

@salkinium
Copy link
Member

I'd still like to merge this, perhaps after clarification about the deprecation, since it solves a real problem without breaking anything, even if we plan to deprecate this at some undetermined point in the future.

@mikewolfram
Copy link
Contributor Author

If you'd ask me, maybe take this PR as is. Even the std::iterator class is marked deprecated, the compiler doesn't warn about it. Once the containers are replaced with STL or ETL, the "problem" would be gone.

@salkinium
Copy link
Member

salkinium commented May 23, 2020

I'm inclined to agree, I was asking @chris-durand to clarify, since he knows more about C++ than me.

@chris-durand
Copy link
Member

You could merge this as is. std::iterator was deprecated because it can be confusing to read if you use all 5 template parameters. This is not an issue here.
It would still be nice to make the iterator in DynamicArray a proper RandomAccessIterator. Otherwise some algorithms that could work (e.g. sort) won't work and others are slower.
I probably wouldn't bother to improve DynamicArray because it is just a less sophisticated version of std::vector. If we need to keep it for compatibility, we could make it a thin wrapper around std::vector.

@mikewolfram
Copy link
Contributor Author

If I'd decide to work on replacing modm with STL/ETL containers, what would be your preferred idea? Add ETL as a module in ext folder, configure it via settings in project.xml file?

And then let the user decide whether to use STL or ETL? Having typedefs to STL or ETL depending on that decision?

@salkinium
Copy link
Member

There are several ways to add ETL: You could do it only in your project, by adding the include path collector value in project.xml:

<lbuild>
  <collectors>
    <collect name="modm:build:path.include">../path/to/etl</collect>
  </collectors>
</lbuild>

Or if you wanted to intergrate it into modm, you would add it into ext/aster/etl and copy out whatever we need with a module.lb named modm:etl. It would perhaps be interesting to create modm:etl:submodule which correlate to the libraries own "subgroups" so that you don't have to copy so many files, but I don't know how well ETL likes being split apart.

modm has a bit of an issue where its use of C++ library code hasn't been updated in a long time, there's currently not a lot of stdc++ library code used (except for template/compile time stuff) and thus not a lot to replace here. A lot of (pointer, size) is passed around manually in the HAL, not sure what the exact implications of using std::span for that would be.

I would like to see more of the C++ std lib integrated into modm, just sometimes a bit unclear what the trade-offs are in terms of speed/code size. That's where I see ETL being useful, albeits it's still targeting C++03 which is getting quite old by now.

@salkinium
Copy link
Member

Oh and definitely no shimming of ETL to be STL compatible, it's already difficult to impossible to actually port the stdlib (Filesystems anyone? No? What about time/calendar? Ok, but threading? RLY?) so I'm not keen on mixing this up even more. Any modules in modm would explicitly depend on :etl:{feature}.

@salkinium salkinium force-pushed the derive_from_std_iterator branch from 09ae44a to f12252b Compare May 24, 2020 05:43
@salkinium salkinium merged commit f12252b into modm-io:develop May 24, 2020
@salkinium
Copy link
Member

If I'd decide to work on replacing modm with STL/ETL containers

Anything come of this?

During my work on #395, I've come to realize just how limited the STL containers are (lacking a fixed size std::queue for example) and how out-of-date our containers are (lacking .emplace and move constructors). Our modm::atomic::Queue only works for interrupt<->cooperative scheduling, not interrupt<->interrupt or even with threads. Ugh, so much work…

@mikewolfram mikewolfram deleted the derive_from_std_iterator branch June 3, 2020 19:25
@mikewolfram
Copy link
Contributor Author

Not yet. I'm busy to meet the next project milestone and have to delay such things until later this year.

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

Successfully merging this pull request may close these issues.

3 participants