This is an archive of the discontinued LLVM Phabricator instance.

Make FileSpec::Dump use FileSpec::GetPath(), not the other way around.
ClosedPublic

Authored by chaoren on May 27 2015, 4:17 PM.

Diff Detail

Event Timeline

chaoren updated this revision to Diff 26646.May 27 2015, 4:17 PM
chaoren retitled this revision from to Make FileSpec::Dump use FileSpec::GetPath(), not the other way around..
chaoren updated this object.
chaoren edited the test plan for this revision. (Show Details)
chaoren added a reviewer: zturner.
chaoren added a subscriber: Unknown Object (MLST).
chaoren updated this object.May 27 2015, 4:17 PM
zturner edited edge metadata.May 27 2015, 4:35 PM

looks good, just some style nitpicks.

include/lldb/Host/FileSpec.h
634

I think this should either be a const member function that operates on m_syntax or a file static function in the .cpp file.

source/Host/common/FileSpec.cpp
626

Why curly braces instead of parentheses?

829

I'm not sure what the style guide says (if anything) but I personally find it a little hard to read lots of "if (x) y;" on a single line. very close together.

chaoren added inline comments.May 27 2015, 4:53 PM
include/lldb/Host/FileSpec.h
634

Yeah, I agree it'd be better if it's contained in the .cpp file. Normalize/DeNormalize (and maybe even GetPathSeparator) should probably also be in there.

source/Host/common/FileSpec.cpp
626

Since we're using C++11, I prefer uniform initialization whenever possible.

829

Yeah, I have no strong opinion on this. Will update.

chaoren updated this revision to Diff 26649.May 27 2015, 5:15 PM
chaoren edited edge metadata.
  • Address review comments.

Looks good.

zturner accepted this revision.May 28 2015, 9:53 AM
zturner edited edge metadata.
This revision is now accepted and ready to land.May 28 2015, 9:53 AM
This revision was automatically updated to reflect the committed changes.