Wrapping WinAPI FindNextVolume as std iterator

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

```
std::set getVolumeNames()
{
class FindVolumeWrapper : public std::iterator
{
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_

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 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.

StackExchange Code Review Q#79481, answer score: 4