On Thu, Apr 11, 2019 at 6:09 PM Thomas Gummerer <t.gummerer@xxxxxxxxx> wrote: > > On 04/10, Matheus Tavares Bernardino wrote: > > > > diff --git a/dir-iterator.h b/dir-iterator.h > > > > index 970793d07a..93646c3bea 100644 > > > > --- a/dir-iterator.h > > > > +++ b/dir-iterator.h > > > > @@ -19,7 +19,7 @@ > > > > * A typical iteration looks like this: > > > > * > > > > * int ok; > > > > - * struct iterator *iter = dir_iterator_begin(path); > > > > + * struct iterator *iter = dir_iterator_begin(path, 0); > > > > > > Outside of this context, we already mentione errorhandling when > > > 'ok != ITER_DONE' in his example. This still can't happen with the > > > way the dir iterator is used here, but it serves as a reminder if > > > people are using the DIR_ITERATOR_PEDANTIC flag. Good. > > > > This made me think again about the documentation saying that > > dir_iterator_abort() and dir_iterator_advance() may return ITER_ERROR, > > but the implementation does not containing these possibilities. > > (Besides when the pedantic flag is used). Maybe the idea was to make > > API-users implement the check for an ITER_ERROR in case dir-iterator > > needs to start returning it in the future. > > Yeah, I think that was the intention. > > > But do you think such a change in dir-iterator is likely to happen? > > Maybe we could just make dir_iterator_abort() be void and remove this > > section from documentation. Then, for dir_iterator_advance() users > > would only need to check for ITER_ERROR if the pedantic flag was given > > at dir-iterator creation... > > Dunno. In a world where we have the pedantic flag, I think only > returning ITER_ERROR if that flag is given might be what we want to > do. I can't think of a reason why we would want to return ITER_ERROR > without the pedantic flag in that case. Ok. I began doing the change, but got stuck in a specific decision. What I was trying to do is: 1) Make dir_iterator_advance() return ITER_ERROR only when the pedantic flag is given; 2) Make dir_iterator_abort() be void. The first change is trivial. But the second is not so easy: Since the [only] current API user defines other iterators on top of dir-iterator, it would require a somehow big surgery on refs/* to make this change. Should I proceed and make the changes at refs/* or should I keep dir_iterator_abort() returning int, although it can never fail? There's also a third option: The only operation that may fail during dir_iterator_abort() is closedir(). But even on dir_iterator_advance(), I'm treating this error as "non-fatal" in the sense that it's not caught by the pedantic flag (although a warning is emitted). I did it like this because it doesn't seem like a major error during dir iteration... But I could change this and make DIR_ITERATOR_PEDANTIC return ITER_ERROR upon closedir() errors for both dir-iterator advance() and abort() functions. What do you think? > Though I think I would change the example the other way in that case, > and pass DIR_ITERATOR_PEDANTIC to 'dir_iterator_begin()', as it would > be easy to forget error handling otherwise, even when it is > necessary. I'd rather err on the side of showing too much error > handling, than having people forget it and having users run into some > odd edge cases in the wild that the tests don't cover. Yes, I agree. > > Also CC-ed Michael in case he has some input
