[libcxx-commits] [PATCH] D91140: [8/N] [libcxx] Fix the preexisting directory_iterator code for windows

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/src/filesystem/directory_iterator.cpp:75
 }
 #else
 
----------------
ldionne wrote:
> I think adding a comment here to signify that this is the Windows branch would add readability.
Sure, I can add that.


================
Comment at: libcxx/src/filesystem/directory_iterator.cpp:121
+    }
+    __stream_ = ::FindFirstFileW((root / "*").c_str(), &__data_);
     if (__stream_ == INVALID_HANDLE_VALUE) {
----------------
ldionne wrote:
> Isn't that a use-after-free? `(root / "*").c_str()` returns a pointer to the internal string held in the temporary object created by `(root / "*")`, unless I'm missing a subtlety here. I don't think the lifetime of objects in an expression used as a function argument is extended automatically.
I don't know the spec bits exactly that allow this, but I'm fairly sure it's supposed to work. An empirical test: https://godbolt.org/z/za8K46

I'm not too familiar with the C++ spec itself to reference it myself, but https://stackoverflow.com/a/584835/3115956 claims that "12.2 Temporary objects" in the C++ standard covers this, saying that the destructors for such temporaries aren't called until at the end of the full-expression.


================
Comment at: libcxx/src/filesystem/filesystem_common.h:21
+#if !defined(_LIBCPP_WIN32API)
 #include <unistd.h>
 #include <sys/stat.h>
----------------
ldionne wrote:
> Please indent `#include`s in between.
Ok, will do that for this block I'm touching here. There's more unindented blocks in the same file, but I'm keeping them untouched here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91140/new/

https://reviews.llvm.org/D91140