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

added: make CFileItemList iterable #11003

Merged
merged 1 commit into from
Jan 21, 2017
Merged

Conversation

notspiff
Copy link
Contributor

@notspiff notspiff commented Nov 28, 2016

This makes CFileItemList iterable, i.e.

for (auto& it : CFileItemListInstance)

@wsnipex wsnipex added the Type: Improvement non-breaking change which improves existing functionality label Nov 28, 2016
Copy link
Member

@Montellese Montellese left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this. I was always too lazy to do it :-/

@@ -723,6 +723,9 @@ class CFileItemList : public CFileItem
const std::string &GetContent() const { return m_content; };

void ClearSortState();

VECFILEITEMS::const_iterator begin() { return m_items.begin(); }
VECFILEITEMS::const_iterator end() { return m_items.end(); }

This comment was marked as spam.

@akva2
Copy link
Contributor

akva2 commented Nov 28, 2016

On purpose. We do not want a mutable iterator, CFileItemList is based on Add () paradigm. The compiler is smart enough but if you want explicit you shall have explicit give me 15mins to get home .

@akva2
Copy link
Contributor

akva2 commented Nov 28, 2016

meh. you are of course completely correct, my left hand had forgotten to commit the const versions which i realized was needed after testing. i thought you were (only) referring to begin() and end() returning const iterators wanting cbegin cend non-const overloads when i wrote ^^ (and obviously i "read" your comment too fast..). i did learn something new though - the compiler will not consider non-const overloads of cbegin|cend at all.

i have kept the code that was there and added the missing cbegin() cend() implementations. if you want mutable iteration, just say so. i gave my reasoning above, it just seemed to break surrounding design to me.

@Montellese
Copy link
Member

No I don't want mutable iterators because it will break stuff that works when using Add() and Remove(). I was just confused about the naming. But I didn't consider that when I wrote my comment so it wasn't 100% correct either.

@hudokkow
Copy link
Member

hudokkow commented Dec 4, 2016

jenkins build this please

@MartijnKaijser
Copy link
Member

Should there be any updates/changes or can this be merged?

@notspiff
Copy link
Contributor Author

notspiff commented Jan 5, 2017

as far as i am concerned it should be as ok. i need the explicit begin() / end() because the compiler is too stupid to constify on its own (i.e. if the CFileItemList is not in a const context it won't consider cbegin() / cend()).

@Paxxi
Copy link
Member

Paxxi commented Jan 7, 2017

jenkins build and merge this please

@Paxxi Paxxi added the v18 Leia label Jan 7, 2017
@Montellese
Copy link
Member

Let's try again. jenkins build and merge this please

@jenkins4kodi jenkins4kodi merged commit 82f9e65 into xbmc:master Jan 21, 2017
@MartijnKaijser MartijnKaijser added this to the L 18.0-alpha1 milestone Jan 21, 2017
@Montellese
Copy link
Member

Just gave it a try and it doesn't work in all cases. E.g. when I have a const CFileItemList I cannot use the for-range because there's no begin() const and end() const (returning a const_iterator). I also checked the implementation of std::vector and there you have

  • begin() and end()
  • begin() const and end() const
  • cbegin() const and cend() const

so we are missing the begin() const and end() const. Mind providing a follow up PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Improvement non-breaking change which improves existing functionality v18 Leia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants