[PATCH] D47604: Add isl C++ list iterators

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.
Herald added a subscriber: steven_wu.

Nice one, at least for lists.

Don't forget to add `[Polly]` into the title for reviews.



================
Comment at: include/polly/Support/ISLTools.h:22-42
+/// A set of list accessors which make the subsequent iterator implementation
+/// easier.
+inline isl::basic_set list_get(isl::basic_set_list list, long idx) {
+  return list.get_basic_set(idx);
+}
+inline isl::set list_get(isl::set_list list, long idx) {
+  return list.get_set(idx);
----------------
[suggestion] These are not meant to be used outside of `struct list_iterator`, correct? Hence you could put them into an anonymous namespace.


================
Comment at: include/polly/Support/ISLTools.h:24
+/// easier.
+inline isl::basic_set list_get(isl::basic_set_list list, long idx) {
+  return list.get_basic_set(idx);
----------------
[serious] `basic_set_list::get_basic_set` etc. use the type `int` for indices, not `long`.

[suggestion] Pass by `const &` could safe a reference counter increment and decrement.

[style] This does not look like LLVM's [[ http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | naming style ]].


================
Comment at: include/polly/Support/ISLTools.h:45
+/// An iterator for isl lists, which enables e.g. C++ foreach loops.
+template <typename list_type, typename element_type> struct list_iterator {
+private:
----------------
[suggestion] did you check whether you could derive from `llvm::iterator_adaptor_base`? It adds some common iterator boilerplate methods.


================
Comment at: include/polly/Support/ISLTools.h:52
+  list_iterator(list_type &list, int position)
+      : list(list), position(position) {}
+
----------------
[suggestion] Could you add assertions to check that `list` is not null and `position` is within the expected range?


================
Comment at: include/polly/Support/ISLTools.h:56
+
+  void operator++() { position++; }
+
----------------
[style] usually the `operator*` returns a value.


================
Comment at: include/polly/Support/ISLTools.h:58
+
+  bool operator!=(const list_iterator &that) {
+    return that.position != position;
----------------
[serious] No `operator==`? Should be added for consistency.


================
Comment at: include/polly/Support/ISLTools.h:63-64
+
+template <typename list_type>
+auto begin(list_type &list)
+    -> list_iterator<list_type, decltype(list_get(list, 0))> {
----------------
[concern] The template is very general and can be instantiated for any type. Could you check that when C++ foreach looks for overloads for `begin`, `end` it does not try to use these functions in case of non-intended type. I fear we could get strange error message for something non-iteratable like "no overload list_size(isl::id)";


================
Comment at: include/polly/Support/ISLTools.h:64
+template <typename list_type>
+auto begin(list_type &list)
+    -> list_iterator<list_type, decltype(list_get(list, 0))> {
----------------
[suggestion] Should this be `const list_type &`?


Repository:
  rPLO Polly

https://reviews.llvm.org/D47604