Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2898753002:
ReadDirectory() added in webrtc/test/testsupport/fileutils.h (Closed)
DescriptionIn order to land https://codereview.webrtc.org/2790933002/ and due to the
ongoing clean-up work (see https://codereview.webrtc.org/2887093002,
https://codereview.webrtc.org/2894583002/ and
https://codereview.webrtc.org/2891923002/), ReadDirectory() has been
added in webrtc/test/testsupport/fileutils.h.
BUG=NOBUG
Review-Url: https://codereview.webrtc.org/2898753002
Cr-Commit-Position: refs/heads/master@{#18369}
Committed: https://chromium.googlesource.com/external/webrtc/+/00b16f4dc89c5b3eee62d69309e678e49359227f
Patch Set 1 : DirectoryIterator from base fileutils #Patch Set 2 : commented lines removed #
Total comments: 2
Patch Set 3 : ReadDirectory() as function, no recursive deletion #
Total comments: 1
Patch Set 4 : win build fix #Patch Set 5 : add trailing sep char to dir paths #
Total comments: 8
Patch Set 6 : comments from Niels addressed #
Total comments: 6
Patch Set 7 : nits #
Total comments: 20
Patch Set 8 : comments from Karl addressed #
Total comments: 4
Patch Set 9 : nits #
Total comments: 3
Patch Set 10 : final changes and win trybot fix gtest #
Dependent Patchsets: Messages
Total messages: 41 (18 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== DirectoryIterator in webrtc/test/testsupport/fileutils.h, RemoveDirRecursively() added BUG= ========== to ========== RemoveDirRecursively() added in webrtc/test/testsupport.fileutils.h. In order to land https://codereview.webrtc.org/2790933002/ and due to the ongoing clean-up work (see https://codereview.webrtc.org/2887093002, https://codereview.webrtc.org/2894583002/ and https://codereview.webrtc.org/2891923002/), RemoveDirRecursively() has been added. BUG=NOBUG ==========
alessiob@webrtc.org changed reviewers: + kjellander@webrtc.org, nisse@webrtc.org
Description was changed from ========== RemoveDirRecursively() added in webrtc/test/testsupport.fileutils.h. In order to land https://codereview.webrtc.org/2790933002/ and due to the ongoing clean-up work (see https://codereview.webrtc.org/2887093002, https://codereview.webrtc.org/2894583002/ and https://codereview.webrtc.org/2891923002/), RemoveDirRecursively() has been added. BUG=NOBUG ========== to ========== RemoveDirRecursively() added in webrtc/test/testsupport.fileutils.h. In order to land https://codereview.webrtc.org/2790933002/ and due to the ongoing clean-up work (see https://codereview.webrtc.org/2887093002, https://codereview.webrtc.org/2894583002/ and https://codereview.webrtc.org/2891923002/), RemoveDirRecursively() has been added. BUG=NOBUG ==========
Hi, As discussed offline, I need to land this CL in order to land others which fail because of ongoing clean-up work from Niels. Cheers, Alessio
lgtm if this is fine with nisse@ https://codereview.webrtc.org/2898753002/diff/40001/webrtc/test/testsupport/f... File webrtc/test/testsupport/fileutils_unittest.cc (right): https://codereview.webrtc.org/2898753002/diff/40001/webrtc/test/testsupport/f... webrtc/test/testsupport/fileutils_unittest.cc:38: void WriteStringInFile(const std::string& what, const std::string& file_path) { what -> contents?
I'm afraid I see a couple of issues.
First, DirectoryIterator is on my kill list, so I'm not happy to see new code
depending on it.
More importantly, I don't think we should have *any* code to recursively delete
a directory hierarchy in webrtc. It's more subtle than appears at first, with
security issues related to symlinks and stat races. If we *really* need that, we
should use library functionality like fts (of BSD origin, I think, available
also on gnu/linux, no idea if it's available on other platforms).
I think it's fine to add something simple to testsupport/fileutils.h to read a
directory, e.g.,
std::vector<std::string> ReadDirectory(
const std::string& dir);
which would return a list of the files in the given directory (with the
directory component prepended, so ReadDirectory("foo") could return, e.g., {
"foo/bar.wav" }.
I'm not sure if ReadDirectory should include all entries, or only those which
are plain files, but for your purposes, you only want the files.
I had a quick look your depending cl, and I see no obvious need for real
recursive deletion. The directories and subdirectories created by the tool
should be few and easily known, right? So they can be deleted explicitly in
depth first order.
A helper function deleting all files in a directory (which is a lot safer than
*recursive* deletion) could be added, and it's pretty trivial given the existing
functions in testsupport and the suggested ReadDirectory function.
for (const std::string &f : ReadDirectory(dir))
RemoveFile(f);
RemoveDir(dir);
If you like to, you could even experiment with some ScopedTmpDirectory class
which does the above on destruction, which would be kind-of neat, and could be
arranged to get depth-first deletion automatically. But my gut feeling is that
it's overengineering.
Description was changed from ========== RemoveDirRecursively() added in webrtc/test/testsupport.fileutils.h. In order to land https://codereview.webrtc.org/2790933002/ and due to the ongoing clean-up work (see https://codereview.webrtc.org/2887093002, https://codereview.webrtc.org/2894583002/ and https://codereview.webrtc.org/2891923002/), RemoveDirRecursively() has been added. BUG=NOBUG ========== to ========== In order to land https://codereview.webrtc.org/2790933002/ and due to the ongoing clean-up work (see https://codereview.webrtc.org/2887093002, https://codereview.webrtc.org/2894583002/ and https://codereview.webrtc.org/2891923002/), ReadDirectory() has been added in webrtc/test/testsupport/fileutils.h. BUG=NOBUG ==========
Hi, Thanks for your answers. PTAL. Alessio https://codereview.webrtc.org/2898753002/diff/60001/webrtc/test/testsupport/f... File webrtc/test/testsupport/fileutils_unittest.cc (right): https://codereview.webrtc.org/2898753002/diff/60001/webrtc/test/testsupport/f... webrtc/test/testsupport/fileutils_unittest.cc:32: static const char kExtension[] = "tmp"; style fix
https://codereview.webrtc.org/2898753002/diff/40001/webrtc/test/testsupport/f... File webrtc/test/testsupport/fileutils_unittest.cc (right): https://codereview.webrtc.org/2898753002/diff/40001/webrtc/test/testsupport/f... webrtc/test/testsupport/fileutils_unittest.cc:38: void WriteStringInFile(const std::string& what, const std::string& file_path) { On 2017/05/22 18:22:20, kjellander_webrtc wrote: > what -> contents? Acknowledged.
https://codereview.webrtc.org/2898753002/diff/100001/webrtc/base/fileutils.h File webrtc/base/fileutils.h (right): https://codereview.webrtc.org/2898753002/diff/100001/webrtc/base/fileutils.h#... webrtc/base/fileutils.h:46: // the first entry in the directory. I don't think edits to the comments in this file really belong in this cl, but if they improve clarity, I don't object. https://codereview.webrtc.org/2898753002/diff/100001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2898753002/diff/100001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:219: if (!DirExists(path)) { return false; } I'm not sure what DirExists return for an empty string. Since you index by |path_len - 1| below, please check explicitly that the string is non-empty. https://codereview.webrtc.org/2898753002/diff/100001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:224: if (path[path_len - 1] != '\\') { path += '\\'; } Use path.back() instead, and delete the |path_len| variable, here and below. (Non-emptyness check still needed, since path.back() on an empty string is equally invalid). https://codereview.webrtc.org/2898753002/diff/100001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:250: if (::stat(std::string(path + dirent->d_name).c_str(), &stat) != 0) { Unclear to me why you call stat (it would make sense if you wanted to return files only). I think the loop would be clearer with only a single call to readdir, instead of the current organization which seems modeled on the FindFirstFile/FindNextFile pattern used on windows. I.e., somthing like dir = ::opendir(); if (!dir) return false; while (struct dirent* dirent = readdir(dir)) { if (name != "." && name != "..") output->emplace_back(...); }
Hi Niels, Thanks a lot! Everything has been addressed. Cheers, Alessio https://codereview.webrtc.org/2898753002/diff/100001/webrtc/base/fileutils.h File webrtc/base/fileutils.h (right): https://codereview.webrtc.org/2898753002/diff/100001/webrtc/base/fileutils.h#... webrtc/base/fileutils.h:46: // the first entry in the directory. On 2017/05/29 07:20:40, nisse-webrtc wrote: > I don't think edits to the comments in this file really belong in this cl, but > if they improve clarity, I don't object. Acknowledged. https://codereview.webrtc.org/2898753002/diff/100001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2898753002/diff/100001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:219: if (!DirExists(path)) { return false; } On 2017/05/29 07:20:40, nisse-webrtc wrote: > I'm not sure what DirExists return for an empty string. Since you index by > |path_len - 1| below, please check explicitly that the string is non-empty. Done. https://codereview.webrtc.org/2898753002/diff/100001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:224: if (path[path_len - 1] != '\\') { path += '\\'; } On 2017/05/29 07:20:40, nisse-webrtc wrote: > Use path.back() instead, and delete the |path_len| variable, here and below. > (Non-emptyness check still needed, since path.back() on an empty string is > equally invalid). Done. https://codereview.webrtc.org/2898753002/diff/100001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:250: if (::stat(std::string(path + dirent->d_name).c_str(), &stat) != 0) { On 2017/05/29 07:20:40, nisse-webrtc wrote: > Unclear to me why you call stat (it would make sense if you wanted to return > files only). > > I think the loop would be clearer with only a single call to readdir, instead of > the current organization which seems modeled on the FindFirstFile/FindNextFile > pattern used on windows. > > I.e., somthing like > > dir = ::opendir(); > if (!dir) return false; > while (struct dirent* dirent = readdir(dir)) { > if (name != "." && name != "..") output->emplace_back(...); > } Right! I just copied and adapted the code from webrtc/base/fileutils.cc. Thanks :)
LGTM, % a few final nits. https://codereview.webrtc.org/2898753002/diff/120001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2898753002/diff/120001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:219: if (path.length() == 0 || !DirExists(path)) return false; On second look, I think the DirExists check here is useless. It should be sufficient to check for errors from FindFirstFile and opendir, which we already do. https://codereview.webrtc.org/2898753002/diff/120001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:220: output->clear(); The comment in the header file just says that strings are added to |output|. So for consistency, either update the comment or delete this call to clear(). Another option is to change the return type to rtc::Optional<std::vector<std::string>>. I'll leave that for your judgement. https://codereview.webrtc.org/2898753002/diff/120001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:248: RTC_DCHECK(dirent); A DCHECK on the loop condition right above seems unnecessary.
https://codereview.webrtc.org/2898753002/diff/120001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2898753002/diff/120001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:219: if (path.length() == 0 || !DirExists(path)) return false; On 2017/05/29 10:56:39, nisse-webrtc wrote: > On second look, I think the DirExists check here is useless. > It should be sufficient to check for errors from FindFirstFile and opendir, > which we already do. Done. https://codereview.webrtc.org/2898753002/diff/120001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:220: output->clear(); On 2017/05/29 10:56:39, nisse-webrtc wrote: > The comment in the header file just says that strings are added to |output|. So > for consistency, either update the comment or delete this call to clear(). > > Another option is to change the return type to > rtc::Optional<std::vector<std::string>>. I'll leave that for your judgement. Done. https://codereview.webrtc.org/2898753002/diff/120001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:248: RTC_DCHECK(dirent); On 2017/05/29 10:56:39, nisse-webrtc wrote: > A DCHECK on the loop condition right above seems unnecessary. Done.
The CQ bit was checked by alessiob@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org, nisse@webrtc.org Link to the patchset: https://codereview.webrtc.org/2898753002/#ps140001 (title: "nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17490)
alessiob@webrtc.org changed reviewers: + kwiberg@webrtc.org
alessiob@webrtc.org changed reviewers: - kjellander@webrtc.org, nisse@webrtc.org
Hi Karl, I need approval from an owner of webrtc/base for this CL. Could you take a look? Cheers, Alessio
Ironically, if you remove the uncalled-for reformatting, you'll no longer be making any changes in webrtc/base/, and thus not need my approval. :-) https://codereview.webrtc.org/2898753002/diff/140001/webrtc/base/fileutils.cc File webrtc/base/fileutils.cc (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/base/fileutils.cc... webrtc/base/fileutils.cc:44: dirent_(nullptr) { Irrelevant cleanup. https://codereview.webrtc.org/2898753002/diff/140001/webrtc/base/fileutils.h File webrtc/base/fileutils.h (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/base/fileutils.h#... webrtc/base/fileutils.h:57: virtual std::string Name() const; Please don't clean up code that your CL otherwise doesn't touch (except in CLs whose sole purpose is such cleanup). It makes reviewing harder, increases the risk of conflicts, makes reverting harder, etc. https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:131: #else // WEBRTC_ANDROID Irrelevant cleanup? https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:219: if (path.length() == 0) return false; Did you git cl format? https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:221: #if defined(WEBRTC_WIN) Please make separate functions for the win and non-win cases. Functions with #ifdefs in their bodies are hard to read. E.g. namespace { #ifdef WEBRTC_WIN bool ReadDirectoryWin(...) {...} #endif } // namespace bool ReadDirectory(...) { #ifdef WEBRTC_WIN return ReadDirectoryWin(...); #endif } https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:247: while (struct dirent* dirent = readdir(dir)) { This is C++---there's no need to say "struct". https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.h (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.h:74: bool ReadDirectory(std::string path, std::vector<std::string>* output); Should the first argument be const std::string&? The function implementation will not typically move from |path|, right? Document the return value. Is it a success/fail boolean? Consider returning Optional<vector<string>> instead of using an output argument, since this is test code where clarity trumps performance. https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils_unittest.cc (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils_unittest.cc:41: void CleanDir(const std::string& dir, size_t* num_deleted_entries) { Return the number of deleted entries instead?
Thanks a lot Karl, everything addressed. If you wish, take a look at CleanDir() in fileutils_unittest.cc since you suggest that the function returns the number of deleted entries but, as far as I know, I can't do that because I use FAIL(). Cheers, Alessio https://codereview.webrtc.org/2898753002/diff/140001/webrtc/base/fileutils.cc File webrtc/base/fileutils.cc (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/base/fileutils.cc... webrtc/base/fileutils.cc:44: dirent_(nullptr) { On 2017/05/29 12:11:35, kwiberg-webrtc wrote: > Irrelevant cleanup. Removed. https://codereview.webrtc.org/2898753002/diff/140001/webrtc/base/fileutils.h File webrtc/base/fileutils.h (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/base/fileutils.h#... webrtc/base/fileutils.h:57: virtual std::string Name() const; On 2017/05/29 12:11:36, kwiberg-webrtc wrote: > Please don't clean up code that your CL otherwise doesn't touch (except in CLs > whose sole purpose is such cleanup). It makes reviewing harder, increases the > risk of conflicts, makes reverting harder, etc. Yup. This was a left over from a previous PS in which I added a method to DirectoryIterator. https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:131: #else // WEBRTC_ANDROID On 2017/05/29 12:11:36, kwiberg-webrtc wrote: > Irrelevant cleanup? Yes. https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:219: if (path.length() == 0) return false; On 2017/05/29 12:11:36, kwiberg-webrtc wrote: > Did you git cl format? Done. https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:221: #if defined(WEBRTC_WIN) On 2017/05/29 12:11:36, kwiberg-webrtc wrote: > Please make separate functions for the win and non-win cases. Functions with > #ifdefs in their bodies are hard to read. E.g. > > namespace { > > #ifdef WEBRTC_WIN > bool ReadDirectoryWin(...) {...} > #endif > > } // namespace > > bool ReadDirectory(...) { > #ifdef WEBRTC_WIN > return ReadDirectoryWin(...); > #endif > } Done. https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:247: while (struct dirent* dirent = readdir(dir)) { On 2017/05/29 12:11:36, kwiberg-webrtc wrote: > This is C++---there's no need to say "struct". Acknowledged. https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.h (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.h:74: bool ReadDirectory(std::string path, std::vector<std::string>* output); On 2017/05/29 12:11:36, kwiberg-webrtc wrote: > Should the first argument be const std::string&? The function implementation > will not typically move from |path|, right? Nope, I need a local copy to add a trailing / (or \) if missing. > > Document the return value. Is it a success/fail boolean? Done. > > Consider returning Optional<vector<string>> instead of using an output argument, > since this is test code where clarity trumps performance. Done. https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils_unittest.cc (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils_unittest.cc:41: void CleanDir(const std::string& dir, size_t* num_deleted_entries) { On 2017/05/29 12:11:36, kwiberg-webrtc wrote: > Return the number of deleted entries instead? Nope. I can't use FAIL() otherwise, right?
lgtm, but see comments https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.h (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.h:74: bool ReadDirectory(std::string path, std::vector<std::string>* output); On 2017/05/29 13:04:48, AleBzk wrote: > On 2017/05/29 12:11:36, kwiberg-webrtc wrote: > > Should the first argument be const std::string&? The function implementation > > will not typically move from |path|, right? > > Nope, I need a local copy to add a trailing / (or \) if missing. OK, that makes sense. > > > > > Document the return value. Is it a success/fail boolean? > > Done. > > > > > Consider returning Optional<vector<string>> instead of using an output > argument, > > since this is test code where clarity trumps performance. > > Done. https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils_unittest.cc (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils_unittest.cc:41: void CleanDir(const std::string& dir, size_t* num_deleted_entries) { On 2017/05/29 13:04:48, AleBzk wrote: > On 2017/05/29 12:11:36, kwiberg-webrtc wrote: > > Return the number of deleted entries instead? > > Nope. I can't use FAIL() otherwise, right? Yes, FAIL() and ASSERT_*() can only be used in functions that return void. However, it still won't work right unless every caller of CleanDir() remembers to call (ASSERT|EXPECT)_NO_FATAL_FAILURE afterwards. This can’t be enforced by the compiler, so it’s usually best not to try. So in subroutines, try to stick to EXCEPT_* if at all possible, and fall back on RTC_CHECK_* if a condition failure would kill the process anyway. https://codereview.webrtc.org/2898753002/diff/160001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2898753002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:84: return rtc::Optional<std::vector<std::string>>(); This check is the same in both versions, so move to the common function? https://codereview.webrtc.org/2898753002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:275: rtc::Optional<std::vector<std::string>> ReadDirectory(const std::string& path) { Wait, didn't you just explain to me why path should be passed by value instead of by const ref? You end up copying it when making the calls below, since they take it by value. (Not that it matters a lot in test code, but for consistency you should probably pass it by const ref everywhere or by value everywhere.)
https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.h (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.h:74: bool ReadDirectory(std::string path, std::vector<std::string>* output); On 2017/05/30 08:28:12, kwiberg-webrtc wrote: > On 2017/05/29 13:04:48, AleBzk wrote: > > On 2017/05/29 12:11:36, kwiberg-webrtc wrote: > > > Should the first argument be const std::string&? The function implementation > > > will not typically move from |path|, right? > > > > Nope, I need a local copy to add a trailing / (or \) if missing. > > OK, that makes sense. > > > > > > > > > Document the return value. Is it a success/fail boolean? > > > > Done. > > > > > > > > Consider returning Optional<vector<string>> instead of using an output > > argument, > > > since this is test code where clarity trumps performance. > > > > Done. > Acknowledged. https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils_unittest.cc (right): https://codereview.webrtc.org/2898753002/diff/140001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils_unittest.cc:41: void CleanDir(const std::string& dir, size_t* num_deleted_entries) { On 2017/05/30 08:28:12, kwiberg-webrtc wrote: > On 2017/05/29 13:04:48, AleBzk wrote: > > On 2017/05/29 12:11:36, kwiberg-webrtc wrote: > > > Return the number of deleted entries instead? > > > > Nope. I can't use FAIL() otherwise, right? > > Yes, FAIL() and ASSERT_*() can only be used in functions that return void. > However, it still won't work right unless every caller of CleanDir() remembers > to call (ASSERT|EXPECT)_NO_FATAL_FAILURE afterwards. This can’t be enforced by > the compiler, so it’s usually best not to try. So in subroutines, try to stick > to EXCEPT_* if at all possible, and fall back on RTC_CHECK_* if a condition > failure would kill the process anyway. Done. https://codereview.webrtc.org/2898753002/diff/160001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2898753002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:84: return rtc::Optional<std::vector<std::string>>(); On 2017/05/30 08:28:12, kwiberg-webrtc wrote: > This check is the same in both versions, so move to the common function? Done. https://codereview.webrtc.org/2898753002/diff/160001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:275: rtc::Optional<std::vector<std::string>> ReadDirectory(const std::string& path) { On 2017/05/30 08:28:12, kwiberg-webrtc wrote: > Wait, didn't you just explain to me why path should be passed by value instead > of by const ref? You end up copying it when making the calls below, since they > take it by value. > > (Not that it matters a lot in test code, but for consistency you should probably > pass it by const ref everywhere or by value everywhere.) Just wanted to avoid an unnecessary extra copy. The OS-dependent functions in the anon namespace make a copy. But good to know that for consistency the same signature helps.
The CQ bit was checked by alessiob@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org, kjellander@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2898753002/#ps180001 (title: "nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/26482) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/25885)
nisse@webrtc.org changed reviewers: + nisse@webrtc.org
https://codereview.webrtc.org/2898753002/diff/180001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2898753002/diff/180001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:273: return ReadDirectoryWin(path); To me, it was more readable with the windows and non-windows versions inlined here.
https://codereview.webrtc.org/2898753002/diff/180001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2898753002/diff/180001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:273: return ReadDirectoryWin(path); On 2017/05/30 12:18:53, nisse-webrtc wrote: > To me, it was more readable with the windows and non-windows versions inlined > here. Really? I generally find that readability is hurt a lot when #ifdefs slice a scope in half.
https://codereview.webrtc.org/2898753002/diff/180001/webrtc/test/testsupport/... File webrtc/test/testsupport/fileutils.cc (right): https://codereview.webrtc.org/2898753002/diff/180001/webrtc/test/testsupport/... webrtc/test/testsupport/fileutils.cc:273: return ReadDirectoryWin(path); On 2017/05/30 12:37:00, kwiberg-webrtc wrote: > On 2017/05/30 12:18:53, nisse-webrtc wrote: > > To me, it was more readable with the windows and non-windows versions inlined > > here. > > Really? I generally find that readability is hurt a lot when #ifdefs slice a > scope in half. At least in this case, when there's clearly two separate implementations, each only a dozen lines. And I don't like the FooWin and FooNoWin names, for a couple of reasons. I'd say, either use the pattern #ifdef WIN32 // BTW, should it be WEBRTC_WIN? rtc::Optional<std::vector<std::string>> ReadDirectory(const std::string& path) { ... } #else rtc::Optional<std::vector<std::string>> ReadDirectory(const std::string& path) { ... } #endif using a single function name, and accept a small amount of code duplication. Or rtc::Optional<std::vector<std::string>> ReadDirectory(const std::string& path) { if (path.empty()) ... std::vector<...> entries; #if WIN32 ... #else ... #endif return rtc::Optional<...>(std::move(entries)); } with some common code collected at the beginning and end of the function body.
The CQ bit was checked by alessiob@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org, kjellander@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2898753002/#ps200001 (title: "final changes and win trybot fix gtest")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1496311074861680,
"parent_rev": "57fa7683a7b86fc4d9f30e264ed0d8b8755e2f6d", "commit_rev":
"00b16f4dc89c5b3eee62d69309e678e49359227f"}
Message was sent while issue was closed.
Description was changed from ========== In order to land https://codereview.webrtc.org/2790933002/ and due to the ongoing clean-up work (see https://codereview.webrtc.org/2887093002, https://codereview.webrtc.org/2894583002/ and https://codereview.webrtc.org/2891923002/), ReadDirectory() has been added in webrtc/test/testsupport/fileutils.h. BUG=NOBUG ========== to ========== In order to land https://codereview.webrtc.org/2790933002/ and due to the ongoing clean-up work (see https://codereview.webrtc.org/2887093002, https://codereview.webrtc.org/2894583002/ and https://codereview.webrtc.org/2891923002/), ReadDirectory() has been added in webrtc/test/testsupport/fileutils.h. BUG=NOBUG Review-Url: https://codereview.webrtc.org/2898753002 Cr-Commit-Position: refs/heads/master@{#18369} Committed: https://chromium.googlesource.com/external/webrtc/+/00b16f4dc89c5b3eee62d6930... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as https://chromium.googlesource.com/external/webrtc/+/00b16f4dc89c5b3eee62d6930... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
