3
\$\begingroup\$

This is my first attempt at creating an STL-style iterator. Is this code ok?

std::set<std::wstring> getVolumeNames()
{
    class FindVolumeWrapper : public std::iterator<std::input_iterator_tag, std::wstring>
    {
        WCHAR        m_curVolName[MAX_PATH];
        HANDLE       m_hFindVol;
        DWORD        m_dwError;
    public:
        // start iterator
        FindVolumeWrapper(bool)
            : m_hFindVol(::FindFirstVolumeW(m_curVolName, ARRAYSIZE(m_curVolName)))
            , m_dwError(::GetLastError())
        {
            if (m_hFindVol == INVALID_HANDLE_VALUE && m_dwError != ERROR_NO_MORE_FILES)
            {
                throw AutoWinError(m_dwError);
            }
            if (m_dwError == ERROR_MORE_DATA)
            {
                m_dwError = ERROR_SUCCESS;
            }
        }

        // end iterator
        FindVolumeWrapper()
            : m_hFindVol(INVALID_HANDLE_VALUE)
            , m_dwError(ERROR_NO_MORE_FILES)
        {
            m_curVolName[0] = L'\0';
        }

        FindVolumeWrapper& GetNextVolume()
        {
            BOOL bSuccess = ::FindNextVolumeW(m_hFindVol, 
                                              m_curVolName, 
                                              ARRAYSIZE(m_curVolName));
            if (!bSuccess)
            {
                m_dwError = ::GetLastError();
                ::FindVolumeClose(m_hFindVol);                
                if (m_dwError != ERROR_NO_MORE_FILES)
                {
                    throw AutoWinError(m_dwError);
                }
            }
            return *this;
        }

        FindVolumeWrapper& operator ++ ()
        {
            return GetNextVolume();
        }

        std::wstring operator * ()
        {
            return m_curVolName;
        }

        // equal if either both good and point to same element, or both faulty
        bool operator == (FindVolumeWrapper const& other)
        {
            return (this->m_dwError == ERROR_SUCCESS 
                    && other.m_dwError == ERROR_SUCCESS 
                    && wcscmp(m_curVolName, other.m_curVolName) == 0)
                 || (this->m_dwError != ERROR_SUCCESS && other.m_dwError != ERROR_SUCCESS);
        }

        bool operator != (FindVolumeWrapper const& other)
        {
            return !operator == (other);
        }
    };

    std::set<std::wstring> volumeNames(FindVolumeWrapper(true), FindVolumeWrapper());

    return volumeNames;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ This question has two good answers about the specs of a Standard-compliant iterator type. \$\endgroup\$ – glampert Feb 3 '15 at 17:48
  • \$\begingroup\$ I upvoted your answer there because all of the "iterator compliance" points are valid for me as well, and it is a great answer. However, I would also welcome a review of the code as it stands, for the purpose it serves now, notwithstanding lacking features to achieve full stl compliance. \$\endgroup\$ – Felix Dombek Feb 3 '15 at 18:07
  • \$\begingroup\$ Sure thing, as soon as time allows, I'll give your code some attention ;) \$\endgroup\$ – glampert Feb 3 '15 at 20:11
4
\$\begingroup\$

This answer by another user to a similar question lists the whole set of features required by the Standard for a fully compatible iterator type. The list is quite long, and you probably won't need all those features, so this comment is mostly FYI.

You have chosen to make your class an Input Iterator, however, it is not fully compliant with the specification for such. It is missing the post-increment (it++) operator and the -> dereference operator, which are important and should at least be added.


The comparison operators are not const, which is a mistake, since they do not mutate member data. The proper signature would be, for instance with operator ==:

bool operator == (FindVolumeWrapper const& other) const;

Also, this calling syntax for operator == is not wrong, but it is unusual (though you've correctly reused it to implement !=):

 bool operator != (FindVolumeWrapper const& other)
 {
    return !operator == (other);
 }

I would expect this instead:

bool operator != (FindVolumeWrapper const& other) const
{
    return !(*this == other);
}

No special syntax, just a plain comparison.


This comparison is hard to read, too many conditions being checked at once:

bool operator == (FindVolumeWrapper const& other)
{
    return (this->m_dwError == ERROR_SUCCESS 
            && other.m_dwError == ERROR_SUCCESS 
            && wcscmp(m_curVolName, other.m_curVolName) == 0)
         || (this->m_dwError != ERROR_SUCCESS && other.m_dwError != ERROR_SUCCESS);
}

Some auxiliary variables would help:

bool operator == (FindVolumeWrapper const& other) const
{
    const bool bothGood = (this->m_dwError == ERROR_SUCCESS) &&
                          (other.m_dwError == ERROR_SUCCESS);
    const bool bothBad  = (this->m_dwError != ERROR_SUCCESS) &&
                          (other.m_dwError != ERROR_SUCCESS);
    const bool sameName = (wcscmp(m_curVolName, other.m_curVolName) == 0);

    return (bothGood && sameName) || bothBad;
}

Yes this will always evaluate all expressions, though the improvement in readability is still worth it in my book. The comparisons are negligible in any modern machine, so I wouldn't worry about that. The string comparison has the potential to be expensive, so if you really mind that, add an if to avoid wcscmp when the first condition is not met.


Instead of defining one constructor that takes a boolean and one that takes no parameters, with the objective of creating the start and end iterators, you could use a more descriptive method by defining two dummy types and using those instead. Example:

// These are empty, we just need the names
struct StartIteratorTag { };
struct EndIteratorTag   { };

class FindVolumeWrapper 
{
public:
    FindVolumeWrapper(StartIteratorTag) { ... }
    FindVolumeWrapper(EndIteratorTag) { ... }
};

std::set<std::wstring> volumeNames(FindVolumeWrapper(StartIteratorTag()), FindVolumeWrapper(EndIteratorTag()));

Your current method is also not bad thought, this is just another alternative.


Finally, this SO question also discusses about iterator types, I recommend a look at it for further reference. And this one compares the Input/Forward Iterator types.

\$\endgroup\$
  • \$\begingroup\$ Thanks, this is really helpful. But I don't understand the Forward vs Input iterator issue. You are of course right in that this is not yet a complete Input iterator, but neither is it currently a Forward iterator. In your link it says that an Input iterator is simpler (a Forward Iterator is an Input iterator with some added stuff) and also that previous copies of a Forward iterator are not invalidated, however I think copies of my iterator are in fact invalidated because the stored FindNextVolume handle will only ever get the next volume. \$\endgroup\$ – Felix Dombek Feb 4 '15 at 16:00
  • \$\begingroup\$ @FelixDombek, I made a mistake, actually, the Forward iterator is an specialization of the Input Iterator, so it is not simpler as I have stated, I'll correct that. This answer explains it well: The difference between an input iterator and a forward iterator is that an input iterator is a "single-pass" iterator but a forward iterator is a "multi-pass" iterator. So basically it means that the Input iter is for a one-shot pass over the collection, while the Forward iter would let you start iteration again if needed. \$\endgroup\$ – glampert Feb 4 '15 at 16:38
  • \$\begingroup\$ In your case, it seems to me that the Input Iterator was indeed the correct choice. \$\endgroup\$ – glampert Feb 4 '15 at 16:39
  • \$\begingroup\$ Thanks again. But actually, how does the suggestion return !(*this == other); work if operator * is overloaded to return the contained string? It seems to work correctly, but I wonder why ... \$\endgroup\$ – Felix Dombek Feb 26 '15 at 16:31
  • \$\begingroup\$ @FelixDombek, good question indeed. You see, this is a pointer to the current object, so in that case, *this would dereference an actual pointer, calling the universal pointer dereference operator. The overloaded operator * for a type is not applied to pointers, but to values that are trying to behave like pointers. A FindVolumeWrapper iterator instance is not a pointer, so we implement operator * for it to make it behave like one. The same goes for ++. You can only increment an object if you define such operator. Did that make sense? \$\endgroup\$ – glampert Feb 27 '15 at 20:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.