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

Jonas Devlieghere via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Jun 13 09:27:58 PDT 2018

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. 



More information about the lldb-commits mailing list