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: > JDevlieghere wrote: > > labath wrote: > > > 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. > > Alright, I see your point, I was only thinking about its uses within the > > FileSpec class. I think we can solve both issues by making it mandatory > > publicly, and have a private function that maintains the current style. > Agreed, maintaining the style for internal uses of `SetFile` makes perfect > sense. This is pretty intrusive so I'll do this in a follow-up commit. 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
