[PATCH] D48084: [FileSpec] Delegate common operations to llvm::sys::path

Jonas Devlieghere via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 12 12:46:51 PDT 2018


JDevlieghere marked an inline comment as done.
JDevlieghere added inline comments.


================
Comment at: source/Utility/FileSpec.cpp:244-246
+  // Only update style if explicitly requested.
+  if (style)
+    m_style = (*style == Style::native) ? GetNativeStyle() : *style;
----------------
labath wrote:
> I don't really have a clear opinion on whether the new behaviour here is better or not, but it any case this sounds like something worth explicitly calling out.
As far as the FileSpec class is concerned this doesn't change anything, we were always explicitly passing the m_style argument which is now implicit. To me this seems strictly better than what is was, were you might accidentally end up with the host path. 

I'll add a comment in the header.


================
Comment at: unittests/Utility/FileSpecTest.cpp:284
     "//",
-    "//a",
     "//a/",
----------------
labath wrote:
> Any particular reason for removing this?
Yup, llvm doesn't consider `C:` or `//net` to be absolute paths (as opposed to `C:\` or `//net/`. While this is debatable, it makes things consistent with drive names on Windows, which is why I added the net tests above. 


Repository:
  rL LLVM

https://reviews.llvm.org/D48084





More information about the llvm-commits mailing list