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

Remove Uses of std::iterator from VTR Libraries #2557

Closed
8 tasks done
AlexandreSinger opened this issue May 24, 2024 · 9 comments
Closed
8 tasks done

Remove Uses of std::iterator from VTR Libraries #2557

AlexandreSinger opened this issue May 24, 2024 · 9 comments

Comments

@AlexandreSinger
Copy link
Contributor

AlexandreSinger commented May 24, 2024

The std::iterator class will be deprecated in C++17:

In CI, the GCC 12 and CLANG 14 build show the deprecation warnings. There are other warnings in these builds, but the std::iterator warnings are blocking most of them.

This is fine right now since the CI is currently on Ubuntu 22.04, which comes with GCC 11.4 built-in; however, in the near future we should upgrade the CI to Ubuntu 24.04, which comes with GCC 13.3 built-in. GCC 13 is not even being tested by CI yet, but it will have the same deprecation warnings (if not these features will actually be deprecated).

We should begin to transition the use of std::iterator into their own classes. The process seems to be pretty standard, it would just require some work since some of the uses of std::iterator are in external libraries.

The current users of std::iterator in VTR are:

  • librrgraph/src/base/rr_node_impl.h image
  • librrgraph/src/base/rr_node_types.h image
  • libvtrutil/src/vtr_array_view.h image
  • libvtrutil/src/vtr_ragged_matrix.h image
  • libvtrutil/src/vtr_vector.h image
  • EXTERNAL/libtatum/libtatum/tatum/tags/TimingTags.hpp image
  • EXTERNAL/capnproto/c++/src/capnp/compat/std-iterator.h image
  • yosys: Yosys contains deprecation warnings for std::iterator which were resolved in recent versions of Yosys. See discussion below.

There may be more, but these were the ones I could find with grep. Once we clear most of these, some other may become more clear from the CI.

@AlexandreSinger
Copy link
Contributor Author

AlexandreSinger commented May 25, 2024

This resource provides a good description of how to define a custom iterator in modern C++:

Their description of the different properties expected to be implemented is quite good:
image

image

As well as a description of why adding these arguments are important:
image

@AlexandreSinger
Copy link
Contributor Author

libvtrutil/src/vtr_array_view.h and libvtrutil/src/vtr_vector.h, beyond just having obviously duplicated code, have this strange comment and style of declaring something called "my_iter". I find this very strange, and I am not sure how to proceed in this case since it appears to rely on the fact that we are using std::iterator here; but I do not think this is necessary.

@vaughnbetz
Copy link
Contributor

Thanks @AlexandreSinger . No strong preference on how you fix this, but I agree it is good to update. Pushing back to libtatum/upstreaming should be easy. capnproto won't be so easy; maybe the latest version of capnproto gets rid of this?

@AlexandreSinger
Copy link
Contributor Author

@vaughnbetz Looks like you were correct. Capnproto fixed this issue 2 years ago:

Screenshot from 2024-05-27 14-03-07

This means that we may need to upgrade our version of capnproto if we want to 100% remove these warnings. Once the other uses of std::iterator are removed we can decide what to do from there. Based on the git history, we are currently on capnproto v0.9.1

In order to resolve this, we would need to rebase ourselves onto capnproto v1.0.0 at least. (v1.0.2 is the latest). I think we should update to the most recent release version (v1.0.2).

@vaughnbetz
Copy link
Contributor

Thanks @AlexandreSinger . Yes, rebasing onto the latest seems like the best approach. Of course, we should change one thing at a time, so sorting the other iterators before trying the upgrade makes sense!

@AlexandreSinger
Copy link
Contributor Author

Through looking through the CI logs, it looks like there were uses of std::iterator in Yosys as well that I did not originally see (blocked by too many warnings).

Currently we are on yosys-0.32. The developers of Yosys fixed this issue in yosys-0.37 (yosys-0.41 is the latest). Looks like we may need to upgrade Yosys as well.

@vaughnbetz I agree, we should change one thing at a time. Hopefully these upgrades go smoothly.

@vaughnbetz
Copy link
Contributor

Sounds good. Upgrading to the latest yosys makes sense. Good topics for the vtr meeting Thursday. We should probably make a table (or slide deck) of planned upgrades and try to assign owners and a sequence.

@AlexandreSinger
Copy link
Contributor Author

After the recent Yosys upgrade in PR #2646 , this issue is finally resolved. Closing.

@vaughnbetz
Copy link
Contributor

Yaay! Big code cleanup :).

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

No branches or pull requests

2 participants