labath accepted this revision. labath 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; ---------------- JDevlieghere wrote: > 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. Yes, but the counterargument to that is: since you're replacing the old filespec with a completely unrelated path, who's to say that the old path syntax is any better fit than "host". I think it would be best here to actually make this argument mandatory and force everyone to think about the correct syntax when constructing the object, but maybe that's a change for another day. ================ Comment at: unittests/Utility/FileSpecTest.cpp:284 "//", - "//a", "//a/", ---------------- JDevlieghere wrote: > 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. Ah, I see.. I can certainly understand why `c:` wouldn't be considered absolute, but I'm not sure the same thing applies to `//net`. However, I think I can live that interpretation right now.. Repository: rL LLVM https://reviews.llvm.org/D48084 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits