Re: [lmi] What does this test do?

[Vadim Zeitlin]

On Mon, 3 May 2021 14:50:05 +0000 Greg Chicares <gchicares@sbcglobal.net> wrote:

GC> Vadim--in a file that hasn't changed substantially since its 2005 original,
GC> I see:
GC> 
GC>     fs::directory_iterator const i(path);
GC>     LMI_TEST(i->exists());
GC> 
GC> and wonder: what was I thinking? What does it mean when that test succeeds?

 I'm glad that you've raised these questions because I was also puzzled by
this, but finally decided it was not worth discussing it.

 To be honest, I strongly suspect that the test was originally supposed to
check whether the path itself exists, not that the path pointed to by the
directory iterator exists. While I can't be really sure that this was the
intention, it would seem to be more logical to want to test whether the
data directory exists, at least prior to, if not instead of, testing that
it is not empty.

GC> If I were to write a comment explaining it, what would I say?

 If I'm right and the intention was just to test for the directory
existence, the code would be self-documenting enough to not even really
need a comment, but if I had to, I would comment it like this:

        // Verify that the data directory does exist.

This seems redundant, which is why I think such comment is not really
necessary in this case.

 If we really want to check that the directory is not empty, then we would
need something like

        // Verify that the data directory contains at least one valid entry.

but the code would need to be changed too.


GC> I think the quoted code means that 'path' names a directory, and that
GC>  - the directory exists, and

 Yes.

GC>  - it contains at least one entry other than '.' and '..', and

 The problem is that the current code doesn't test for this. To check for
this it would need to do

        LMI_TEST( i != fs::directory_iterator{} );

GC>  - the first pointed-to entry refers to an existing filesystem object.
GC> That seems like a strange way of testing that the directory
GC> is not empty.

 I don't see any other way to do it using std::fs API.

GC> And, if it is empty, wouldn't dereferencing 'i' be UB?

 Yes, it would, which is why we need to check that the iterator is valid,
as I suggest above, first.
 

GC> I'd like to replace those lines with something I can understand, e.g.:
GC>   LMI_TEST(is_directory(path));
GC> Poring over the Standard leads me to believe is_directory() is
GC> implemented in terms of status(), which behaves as if implemented
GC> in terms of posix stat(). And we can't 'stat' a file that doesn't
GC> exist, AFAIK, so if 'true == is_directory(path)', then the directory
GC> necessarily exists. Therefore, it's not necessary to add an explicit
GC> existence test:
GC> 
GC>     LMI_TEST(is_directory(path)); // Success implies existence.
GC> //  LMI_TEST(path.exists());      // Unnecessary.
GC> 
GC> and if we really do want to be sure the directory is not empty:
GC> 
GC>     LMI_TEST(is_directory(path)); //
GC>     LMI_TEST(!is_empty(path));    // Of dubious value.
GC> 
GC> but I don't think nonemptiness is an invariant that we should
GC> try to enforce here: if running this unit test is the first
GC> thing we do in a brand-new installation, we might not have
GC> populated this directory yet.

 I wasn't sure about whether the data directory could be empty, but if it
can, then I'm even more certain that my existing hypothesis was correct and
that the original intention was really to just check for the directory
existence, not the existence of any entries in it.

 And I agree that checking is_directory() is even better than using
exists(), and because status() follows sym links, this should work
correctly even in my custom environment where this directory may be a
symlink.

 It looks like you are going to change it yourself and I don't need to
actually do anything here, but please let me know if I'm wrong and you'd
like me to make any patches fixing this.

 Thanks,
VZ

pgpw8zGfeeM2N.pgp
Description: PGP signature