[PATCH] D27233: [libFuzzer] Diff 3 - Split IO implementation for Windows and Posix.

[Marcos Pividori via Phabricator via llvm-commits]

mpividori added inline comments.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:28
+bool IsFile(const std::string &Path) {
+  DWORD Att = GetFileAttributesA(Path.c_str());
+
----------------
amccarth wrote:
> This is OK as long as you know the path names will contain only characters in the user's code page (which isn't UTF-8).
> 
> If the path name being passed in is UTF-8, then you'll need to convert it to a `std::wstring` and call the -W versions instead of the -A versions of these file functions.
> 
> The -W versions also make it possible to deal with very long path names.
@amccarth thanks for your comments. Yes, we plan to modify the code in the future to use `std::wstring`, as `Support` library does.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:83
+  // Get the first directory entry.
+  WIN32_FIND_DATAA FirstFind;
+  HANDLE FindHandle(FindFirstFileA(Path.c_str(), &FirstFind));
----------------
amccarth wrote:
> `FirstFind` doesn't seem like a great name, since it's also used for the subsequent files.  `FindInfo` or something like that is probably better.
Ok, I will change that.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:100
+
+    DWORD Att = GetFileAttributesA(FileName.c_str());
+
----------------
amccarth wrote:
> You don't need to call GetFileAttributesA.  The attributes are already available in the WIN32_FIND_DATA.
@amccarth you are right. I will modify this.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:109
+
+    if (Att & FILE_ATTRIBUTE_DIRECTORY)
+      ListFilesInDirRecursive(FileName, Epoch, V, false);
----------------
amccarth wrote:
> You could put the test for `.` and `..` in this block, which should make things faster.
@amccarth Ok.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:110
+    if (Att & FILE_ATTRIBUTE_DIRECTORY)
+      ListFilesInDirRecursive(FileName, Epoch, V, false);
+    else if (IsFile(FileName))
----------------
amccarth wrote:
> I generally prefer an iterative solution instead of recursive.  I've found it's slightly faster to iterate one directory completely before descending to a subdirectory.  It also uses fewer system resources, as you have only one FindHandle open at a time.  But this is probably fine, and I don't know if the caller is expecting the files in the recursive order.
> 
> The iterative solution is to have a queue of directories.  Each time you find a new directory, push it onto the queue.  Iterate until the queue is empty.
@amccarth thanks. I adapted the previous implementation in libFuzzer for Posix, which was recursive. I don't think the order of files matters here, so I can change it to consider an iterative solution. I will add that changes in a new diff.


Repository:
  rL LLVM

https://reviews.llvm.org/D27233