[PATCH] D109794: [Sanitizer] Allow setting the report path to create directory

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_common.h:263
 
+#if !SANITIZER_FUCHSIA
+int CreateDir(const char *pathname);
----------------
can we avoid #if?
we don't have one e.g. for GetProcessName


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_file.cpp:78
 
+static void RecursiveMkdir(char *path) {
+  int i;
----------------
Maybe we can add ReportFile::SetReportPath unittest here: 
sanitizer_common/tests/sanitizer_common_test.cpp

The function is little bit unconventional and does not create path for the last component. Maybe rename into something RecursiveCreateParentDirs(char* path)


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_file.cpp:82
+
+  for (i = start; path[i] != '\0'; ++i) {
+    char save = path[i];
----------------
We don't need two vars outside the loop.
Also if we start with 1 we can overflow on empty string.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_file.cpp:84
+    char save = path[i];
+    if (IsPathSeparator(path[i]))
+      continue;
----------------
if (!IsPathSeparator) ?




================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp:154
 
+int CreateDir(const char *pathname) { return mkdir(pathname, 0755); }
+
----------------
int -> bool, to avoid exposing platform specific error errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109794