File system path resolving

src/DefaultFileSystem.cpp

 #include <AdblockPlus/DefaultFileSystem.h>
 #include <cstdio>
 #include <cstring>
 #include <fstream>
 #include <stdexcept>
 
 #include <cerrno>
 #include <sys/types.h>
 #include <sys/stat.h>
 
 #ifdef _WIN32
 #ifndef S_ISDIR
 #define S_ISDIR(mode)  (((mode) & S_IFMT) == S_IFDIR)
 #include <Shlobj.h>
+#include <Shlwapi.h>
 #endif
 
 #ifndef S_ISREG
 #define S_ISREG(mode)  (((mode) & S_IFMT) == S_IFREG)
 #endif
 #endif
 
 #include "../src/Utils.h"
 
 using namespace AdblockPlus;
@@ skipping 29 matching lines @@
   if (rename(fromPath.c_str(), toPath.c_str()))
     throw RuntimeErrorWithErrno("Failed to move " + fromPath + " to " + toPath);
 }
 
 void DefaultFileSystem::Remove(const std::string& path)
 {
   if (remove(path.c_str()))
     throw RuntimeErrorWithErrno("Failed to remove " + path);
 }
 
+/*
+ * In order to get millisecond resolution for modification times, it's necessary to use the 'stat' structure defined in
+ * POSIX 2008, which has 'struct timespec st_mtim' instead of 'time_t st_mtime'. Use "#define _POSIX_C_SOURCE 200809L" 
+ * before the headers to invoke. The trouble is that not all systems may have this available, a category that includes 
+ * MS Windows, and so we'll need multiple implementations.
+ */
 FileSystem::StatResult DefaultFileSystem::Stat(const std::string& path) const
 {
 #ifdef WIN32
   struct _stat nativeStat;
   const int failure = _stat(path.c_str(), &nativeStat);
 #else
   struct stat nativeStat;
   const int failure = stat(path.c_str(), &nativeStat);
 #endif
   if (failure) {
     if (errno == ENOENT)
       return FileSystem::StatResult();
     throw RuntimeErrorWithErrno("Unable to stat " + path);
   }
   FileSystem::StatResult result;
   result.exists = true;
   result.isFile = S_ISREG(nativeStat.st_mode);
   result.isDirectory = S_ISDIR(nativeStat.st_mode);
   result.lastModified = static_cast<int64_t>(nativeStat.st_mtime) * 1000;
   return result;
 }
 
 std::string DefaultFileSystem::Resolve(const std::string& path) const
 {
-  return basePath + "\\" + path;

[Felix Dahlke, 2013/04/30 09:14:52]

\\ won't work on UNIX, but I think / should work everywhere.

[Wladimir Palant, 2013/04/30 09:29:50]

1) This path separator is Windows-specific. Please define it in a cross-platform
way, e.g.:

#ifdef _WIN32
#define PATH_SEPARATOR '\\'
#else
#define PATH_SEPARATOR '/'
#endif

And use PATH_SEPARATOR here.

2) What if path is already an absolute path? You need to call PathIsRelative()
on Windows and check first whether the path starts with / on all other platforms
- absolute paths should be returned unchanged.

3) What if SetBasePath() was never called (basePath is empty)? We should return
the path unchanged then, the caller shouldn't be required to call SetBasePath().

4) While we are at it, maybe we should already prefix Windows paths with \\?\
unless PathIsUNC() returns true. This makes sure we don't hit the MAX_PATH
limit.

+  if (basePath == "")
+  {
+    return path;
+  }
+  else
+  {
+    if (PathIsRelative(Utils::ToUTF16String(path, path.length()).c_str()))

[Wladimir Palant, 2013/05/06 06:18:30]

This is Windows-specific and won't compile on other systems, should be:

#ifdef _WIN32
  if (PathIsRelative(Utils::ToUTF16String(path, path.length()).c_str()))
#else
  if (path.length() && *path.begin() == PATH_SEPARATOR)
#endif

[Wladimir Palant, 2013/05/06 06:19:51]

Oops, exactly the other way round of course:

   if (!path.length() || *path.begin() != PATH_SEPARATOR)

+    {
+      return basePath + PATH_SEPARATOR + path;
+    }
+    else
+    {
+      return path;
+    }
+  }
 }
 
 void DefaultFileSystem::SetBasePath(const std::string& path)
 {
+  basePath = path;
 
-  if (path.rfind('\\') == (path.length() - 1))

[Felix Dahlke, 2013/04/30 09:14:52]

How about if (*path.rbegin() == '\\')?

This also needs to check for / on Linux, maybe both on Windows since / is AFAIK
generally a legal path separator in Windows.

[Wladimir Palant, 2013/04/30 09:29:50]

Please search for PATH_SEPARATOR here as well, not for Windows-specific path
separators. Btw, I would rather write that check as:

  if (path.length() && path[path.length() - 1] == PATH_SEPARATOR)

But that's pretty much a matter of taste.

+  if ((*basePath.rbegin() == PATH_SEPARATOR))

[Wladimir Palant, 2013/05/06 06:18:30]

Nit: You seem to have an extra pair parentheses here.

   {
-    basePath = path.substr(0, path.length() - 1);

[Felix Dahlke, 2013/04/30 09:14:52]

Never used this, but basePath.pop_back(); seems to be possible in C++0x.

-  }
-  else
-  {
-    basePath = path;
+    basePath.pop_back();
   }
 }