[Lldb-commits] [lldb] r330568 - [CMake] Gate 'dsymutil' dependency

2018-04-23 Thread Jonas Devlieghere via lldb-commits
Author: jdevlieghere
Date: Mon Apr 23 01:44:06 2018
New Revision: 330568

URL: http://llvm.org/viewvc/llvm-project?rev=330568&view=rev
Log:
[CMake] Gate 'dsymutil' dependency

Only add `dsymutil` as a test dependency when this is not a stand-alone
build (LLDB_BUILT_STANDALONE).

Modified:
lldb/trunk/test/CMakeLists.txt

Modified: lldb/trunk/test/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/test/CMakeLists.txt?rev=330568&r1=330567&r2=330568&view=diff
==
--- lldb/trunk/test/CMakeLists.txt (original)
+++ lldb/trunk/test/CMakeLists.txt Mon Apr 23 01:44:06 2018
@@ -13,7 +13,7 @@ function(add_python_test_target name tes
 )
 endfunction()
 
-set(LLDB_TEST_DEPS lldb dsymutil)
+set(LLDB_TEST_DEPS lldb)
 
 # darwin-debug is an hard dependency for the testsuite.
 if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
@@ -35,7 +35,7 @@ if(TARGET lldb-mi)
 endif()
 
 if(NOT LLDB_BUILT_STANDALONE)
-  list(APPEND LLDB_TEST_DEPS yaml2obj)
+  list(APPEND LLDB_TEST_DEPS yaml2obj dsymutil)
 endif()
 
 if(TARGET liblldb)


___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [lldb] r330578 - llgs-tests: Increase packet time out

2018-04-23 Thread Pavel Labath via lldb-commits
Author: labath
Date: Mon Apr 23 04:22:44 2018
New Revision: 330578

URL: http://llvm.org/viewvc/llvm-project?rev=330578&view=rev
Log:
llgs-tests: Increase packet time out

The default packet timeout of 1 second is a bit too small for these
tests, particularly as they are working in ack-mode, which means they
need to fit twice as many packets into the timeslot.

This does not seem to be a problem on the bots, but for some people
these tests are timing out regularly. I can't be sure increasing this
will solve their problem, but this does seem like a likely culprit.

Modified:
lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp

Modified: lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp?rev=330578&r1=330577&r2=330578&view=diff
==
--- lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp (original)
+++ lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp Mon Apr 23 
04:22:44 2018
@@ -29,6 +29,7 @@ using namespace llgs_tests;
 
 TestClient::TestClient(std::unique_ptr Conn) {
   SetConnection(Conn.release());
+  SetPacketTimeout(std::chrono::seconds(10));
 
   SendAck(); // Send this as a handshake.
 }


___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45949: [dotest] Make the set of tests independent of the test configuration

2018-04-23 Thread Pavel Labath via Phabricator via lldb-commits
labath created this revision.
labath added reviewers: JDevlieghere, aprantl.
Herald added a subscriber: eraman.

In the magic test duplicator, we were making the decision whether to
create a test variant based on the compiler and the target platform.
This meant that the set of known tests was different for each test
configuration.

This patch makes the set of generated test variants static and handles
the skipping via runtime checks instead. This is more consistent with
how we do other test-skipping decision (e.g. for libc++ tests), and
makes it easier to expose the full set of tests to lit, which now does
not need to know anything about what things can potentially cause tests
to appear or disappear.


https://reviews.llvm.org/D45949

Files:
  packages/Python/lldbsuite/test/dotest.py
  packages/Python/lldbsuite/test/lldbinline.py
  packages/Python/lldbsuite/test/lldbtest.py

Index: packages/Python/lldbsuite/test/lldbtest.py
===
--- packages/Python/lldbsuite/test/lldbtest.py
+++ packages/Python/lldbsuite/test/lldbtest.py
@@ -1736,65 +1736,29 @@
 for attrname, attrvalue in attrs.items():
 if attrname.startswith("test") and not getattr(
 attrvalue, "__no_debug_info_test__", False):
-target_platform = lldb.DBG.GetSelectedPlatform(
-).GetTriple().split('-')[2]
 
 # If any debug info categories were explicitly tagged, assume that list to be
 # authoritative.  If none were specified, try with all debug
 # info formats.
-all_dbginfo_categories = set(
-test_categories.debug_info_categories) - set(configuration.skipCategories)
+all_dbginfo_categories = set(test_categories.debug_info_categories)
 categories = set(
 getattr(
 attrvalue,
 "categories",
 [])) & all_dbginfo_categories
 if not categories:
 categories = all_dbginfo_categories
 
-supported_categories = [
-x for x in categories if test_categories.is_supported_on_platform(
-x, target_platform, configuration.compiler)]
-
-if "dsym" in supported_categories:
-@decorators.add_test_categories(["dsym"])
+for cat in categories:
+@decorators.add_test_categories([cat])
 @wraps(attrvalue)
-def dsym_test_method(self, attrvalue=attrvalue):
+def test_method(self, attrvalue=attrvalue):
 return attrvalue(self)
-dsym_method_name = attrname + "_dsym"
-dsym_test_method.__name__ = dsym_method_name
-dsym_test_method.debug_info = "dsym"
-newattrs[dsym_method_name] = dsym_test_method
 
-if "dwarf" in supported_categories:
-@decorators.add_test_categories(["dwarf"])
-@wraps(attrvalue)
-def dwarf_test_method(self, attrvalue=attrvalue):
-return attrvalue(self)
-dwarf_method_name = attrname + "_dwarf"
-dwarf_test_method.__name__ = dwarf_method_name
-dwarf_test_method.debug_info = "dwarf"
-newattrs[dwarf_method_name] = dwarf_test_method
-
-if "dwo" in supported_categories:
-@decorators.add_test_categories(["dwo"])
-@wraps(attrvalue)
-def dwo_test_method(self, attrvalue=attrvalue):
-return attrvalue(self)
-dwo_method_name = attrname + "_dwo"
-dwo_test_method.__name__ = dwo_method_name
-dwo_test_method.debug_info = "dwo"
-newattrs[dwo_method_name] = dwo_test_method
-
-if "gmodules" in supported_categories:
-@decorators.add_test_categories(["gmodules"])
-@wraps(attrvalue)
-def gmodules_test_method(self, attrvalue=attrvalue):
-return attrvalue(self)
-gmodules_method_name = attrname + "_gmodules"
-gmodules_test_method.__name__ = gmodules_method_name
-gmodules_test_method.debug_info = "gmodules"
-newattrs[gmodules_method_name] = gmodules_test_method
+method_name = attrname + "_" + cat
+test_method.__name__ = method_name
+test_method.debug_info = cat
+newattrs[method_name] = test_method
 
 else:
 newattrs[attrname] = attrvalue
Index: packages/Python/lldbsuite/test/lldbinline.py

[Lldb-commits] [lldb] r330617 - [CMake] Add the missing `dsymutil` dependency when running tests.

2018-04-23 Thread Davide Italiano via lldb-commits
Author: davide
Date: Mon Apr 23 10:06:55 2018
New Revision: 330617

URL: http://llvm.org/viewvc/llvm-project?rev=330617&view=rev
Log:
[CMake] Add the missing `dsymutil` dependency when running tests.

Modified:
lldb/trunk/lit/CMakeLists.txt

Modified: lldb/trunk/lit/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/CMakeLists.txt?rev=330617&r1=330616&r2=330617&view=diff
==
--- lldb/trunk/lit/CMakeLists.txt (original)
+++ lldb/trunk/lit/CMakeLists.txt Mon Apr 23 10:06:55 2018
@@ -38,6 +38,7 @@ configure_lit_site_cfg(
 
 set(LLDB_TEST_DEPS
   LLDBUnitTests
+  dsymutil
   lldb
   lldb-test
   llvm-config


___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Greg Clayton via Phabricator via lldb-commits
clayborg created this revision.
clayborg added reviewers: labath, jingham, davide, pranavb.
Herald added subscribers: JDevlieghere, aprantl.

Always normalizing lldb_private::FileSpec paths will help us get a consistent 
results from comparisons when setting breakpoints and when looking for source 
files. This also removes a lot of complexity from the comparison routines. 
Modified the DWARF line table parser to use the normalized compile unit 
directory if needed.


https://reviews.llvm.org/D45977

Files:
  include/lldb/Core/FileSpecList.h
  include/lldb/Utility/FileSpec.h
  lldb.xcodeproj/xcshareddata/xcschemes/darwin-debug.xcscheme
  lldb.xcodeproj/xcshareddata/xcschemes/desktop.xcscheme
  lldb.xcodeproj/xcshareddata/xcschemes/lldb-gtest.xcscheme
  source/Breakpoint/BreakpointResolverFileLine.cpp
  source/Core/FileSpecList.cpp
  source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
  source/Plugins/SymbolFile/DWARF/DWARFDebugLine.h
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  source/Symbol/CompileUnit.cpp
  source/Symbol/Declaration.cpp
  source/Utility/FileSpec.cpp
  unittests/Utility/FileSpecTest.cpp

Index: unittests/Utility/FileSpecTest.cpp
===
--- unittests/Utility/FileSpecTest.cpp
+++ unittests/Utility/FileSpecTest.cpp
@@ -54,17 +54,14 @@
 
   FileSpec fs_posix_trailing_slash("/foo/bar/", false,
FileSpec::ePathSyntaxPosix);
-  EXPECT_STREQ("/foo/bar/.", fs_posix_trailing_slash.GetCString());
-  EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetDirectory().GetCString());
-  EXPECT_STREQ(".", fs_posix_trailing_slash.GetFilename().GetCString());
+  EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetCString());
+  EXPECT_STREQ("/foo", fs_posix_trailing_slash.GetDirectory().GetCString());
+  EXPECT_STREQ("bar", fs_posix_trailing_slash.GetFilename().GetCString());
 
   FileSpec fs_windows_trailing_slash("F:\\bar\\", false,
  FileSpec::ePathSyntaxWindows);
-  EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString());
-  // EXPECT_STREQ("F:\\bar",
-  // fs_windows_trailing_slash.GetDirectory().GetCString()); // It returns
-  // "F:/bar"
-  EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString());
+  EXPECT_STREQ("F:\\bar", fs_windows_trailing_slash.GetCString());
+  EXPECT_STREQ("bar", fs_windows_trailing_slash.GetFilename().GetCString());
 }
 
 TEST(FileSpecTest, AppendPathComponent) {
@@ -131,32 +128,13 @@
   EXPECT_STREQ("F:\\bar", fs_windows_root.GetCString());
 }
 
-static void Compare(const FileSpec &one, const FileSpec &two, bool full_match,
-bool remove_backup_dots, bool result) {
-  EXPECT_EQ(result, FileSpec::Equal(one, two, full_match, remove_backup_dots))
-  << "File one: " << one.GetCString() << "\nFile two: " << two.GetCString()
-  << "\nFull match: " << full_match
-  << "\nRemove backup dots: " << remove_backup_dots;
-}
-
 TEST(FileSpecTest, EqualSeparator) {
   FileSpec backward("C:\\foo\\bar", false, FileSpec::ePathSyntaxWindows);
   FileSpec forward("C:/foo/bar", false, FileSpec::ePathSyntaxWindows);
   EXPECT_EQ(forward, backward);
-
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
-  Compare(forward, backward, full_match, remove_backup_dots, match);
-  Compare(forward, backward, full_match, !remove_backup_dots, match);
-  Compare(forward, backward, !full_match, remove_backup_dots, match);
-  Compare(forward, backward, !full_match, !remove_backup_dots, match);
 }
 
 TEST(FileSpecTest, EqualDotsWindows) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair tests[] = {
   {R"(C:\foo\bar\baz)", R"(C:\foo\foo\..\bar\baz)"},
   {R"(C:\bar\baz)", R"(C:\foo\..\bar\baz)"},
@@ -170,18 +148,11 @@
   for (const auto &test : tests) {
 FileSpec one(test.first, false, FileSpec::ePathSyntaxWindows);
 FileSpec two(test.second, false, FileSpec::ePathSyntaxWindows);
-EXPECT_NE(one, two);
-Compare(one, two, full_match, remove_backup_dots, match);
-Compare(one, two, full_match, !remove_backup_dots, !match);
-Compare(one, two, !full_match, remove_backup_dots, match);
-Compare(one, two, !full_match, !remove_backup_dots, !match);
+EXPECT_EQ(one, two);
   }
 }
 
 TEST(FileSpecTest, EqualDotsPosix) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair tests[] = {
   {R"(/foo/bar/baz)", R"(/foo/foo/../bar/baz)"},
   {R"(/bar/baz)", R"(/foo/../bar/baz)"},
@@ -193,18 +164,11 @@
   for (const auto &test : tests) {
 FileSpec one(test.first, false, FileSpec::ePathSyntaxPosix);
 FileSpec two(test.second, false, FileSpec::ePathSyntaxPosix);
-EXPECT_NE(one, two);
-Compare(one, two, full_match, remove_backup_dots, match);
-Co

[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Greg Clayton via Phabricator via lldb-commits
clayborg updated this revision to Diff 143608.
clayborg added a comment.

Remove unwanted file changes.


https://reviews.llvm.org/D45977

Files:
  include/lldb/Core/FileSpecList.h
  include/lldb/Utility/FileSpec.h
  source/Breakpoint/BreakpointResolverFileLine.cpp
  source/Core/FileSpecList.cpp
  source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
  source/Plugins/SymbolFile/DWARF/DWARFDebugLine.h
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  source/Symbol/CompileUnit.cpp
  source/Symbol/Declaration.cpp
  source/Utility/FileSpec.cpp
  unittests/Utility/FileSpecTest.cpp

Index: unittests/Utility/FileSpecTest.cpp
===
--- unittests/Utility/FileSpecTest.cpp
+++ unittests/Utility/FileSpecTest.cpp
@@ -54,17 +54,14 @@
 
   FileSpec fs_posix_trailing_slash("/foo/bar/", false,
FileSpec::ePathSyntaxPosix);
-  EXPECT_STREQ("/foo/bar/.", fs_posix_trailing_slash.GetCString());
-  EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetDirectory().GetCString());
-  EXPECT_STREQ(".", fs_posix_trailing_slash.GetFilename().GetCString());
+  EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetCString());
+  EXPECT_STREQ("/foo", fs_posix_trailing_slash.GetDirectory().GetCString());
+  EXPECT_STREQ("bar", fs_posix_trailing_slash.GetFilename().GetCString());
 
   FileSpec fs_windows_trailing_slash("F:\\bar\\", false,
  FileSpec::ePathSyntaxWindows);
-  EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString());
-  // EXPECT_STREQ("F:\\bar",
-  // fs_windows_trailing_slash.GetDirectory().GetCString()); // It returns
-  // "F:/bar"
-  EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString());
+  EXPECT_STREQ("F:\\bar", fs_windows_trailing_slash.GetCString());
+  EXPECT_STREQ("bar", fs_windows_trailing_slash.GetFilename().GetCString());
 }
 
 TEST(FileSpecTest, AppendPathComponent) {
@@ -131,32 +128,13 @@
   EXPECT_STREQ("F:\\bar", fs_windows_root.GetCString());
 }
 
-static void Compare(const FileSpec &one, const FileSpec &two, bool full_match,
-bool remove_backup_dots, bool result) {
-  EXPECT_EQ(result, FileSpec::Equal(one, two, full_match, remove_backup_dots))
-  << "File one: " << one.GetCString() << "\nFile two: " << two.GetCString()
-  << "\nFull match: " << full_match
-  << "\nRemove backup dots: " << remove_backup_dots;
-}
-
 TEST(FileSpecTest, EqualSeparator) {
   FileSpec backward("C:\\foo\\bar", false, FileSpec::ePathSyntaxWindows);
   FileSpec forward("C:/foo/bar", false, FileSpec::ePathSyntaxWindows);
   EXPECT_EQ(forward, backward);
-
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
-  Compare(forward, backward, full_match, remove_backup_dots, match);
-  Compare(forward, backward, full_match, !remove_backup_dots, match);
-  Compare(forward, backward, !full_match, remove_backup_dots, match);
-  Compare(forward, backward, !full_match, !remove_backup_dots, match);
 }
 
 TEST(FileSpecTest, EqualDotsWindows) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair tests[] = {
   {R"(C:\foo\bar\baz)", R"(C:\foo\foo\..\bar\baz)"},
   {R"(C:\bar\baz)", R"(C:\foo\..\bar\baz)"},
@@ -170,18 +148,11 @@
   for (const auto &test : tests) {
 FileSpec one(test.first, false, FileSpec::ePathSyntaxWindows);
 FileSpec two(test.second, false, FileSpec::ePathSyntaxWindows);
-EXPECT_NE(one, two);
-Compare(one, two, full_match, remove_backup_dots, match);
-Compare(one, two, full_match, !remove_backup_dots, !match);
-Compare(one, two, !full_match, remove_backup_dots, match);
-Compare(one, two, !full_match, !remove_backup_dots, !match);
+EXPECT_EQ(one, two);
   }
 }
 
 TEST(FileSpecTest, EqualDotsPosix) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair tests[] = {
   {R"(/foo/bar/baz)", R"(/foo/foo/../bar/baz)"},
   {R"(/bar/baz)", R"(/foo/../bar/baz)"},
@@ -193,18 +164,11 @@
   for (const auto &test : tests) {
 FileSpec one(test.first, false, FileSpec::ePathSyntaxPosix);
 FileSpec two(test.second, false, FileSpec::ePathSyntaxPosix);
-EXPECT_NE(one, two);
-Compare(one, two, full_match, remove_backup_dots, match);
-Compare(one, two, full_match, !remove_backup_dots, !match);
-Compare(one, two, !full_match, remove_backup_dots, match);
-Compare(one, two, !full_match, !remove_backup_dots, !match);
+EXPECT_EQ(one, two);
   }
 }
 
 TEST(FileSpecTest, EqualDotsPosixRoot) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair tests[] = {
   {R"(/)", R"(/..)"},
   {R"(/)", R"(/.)"},
@@ -214,11 +178,7 @@
   for (const auto &test : tests) {
 FileSpec one(test.first, false, FileS

[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl added inline comments.



Comment at: lldb.xcodeproj/xcshareddata/xcschemes/darwin-debug.xcscheme:29
   selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.GDB"
+  language = ""
   shouldUseLaunchSchemeArgsEnv = "YES">

Is this an accidental change or is it relevant to the patch?



Comment at: lldb.xcodeproj/xcshareddata/xcschemes/desktop.xcscheme:68
+ 

This looks like an accidental commit?


https://reviews.llvm.org/D45977



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Greg Clayton via Phabricator via lldb-commits
clayborg updated this revision to Diff 143609.
clayborg added a comment.

Remove commented out code.


https://reviews.llvm.org/D45977

Files:
  include/lldb/Core/FileSpecList.h
  include/lldb/Utility/FileSpec.h
  source/Breakpoint/BreakpointResolverFileLine.cpp
  source/Core/FileSpecList.cpp
  source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
  source/Plugins/SymbolFile/DWARF/DWARFDebugLine.h
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  source/Symbol/CompileUnit.cpp
  source/Symbol/Declaration.cpp
  source/Utility/FileSpec.cpp
  unittests/Utility/FileSpecTest.cpp

Index: unittests/Utility/FileSpecTest.cpp
===
--- unittests/Utility/FileSpecTest.cpp
+++ unittests/Utility/FileSpecTest.cpp
@@ -54,17 +54,14 @@
 
   FileSpec fs_posix_trailing_slash("/foo/bar/", false,
FileSpec::ePathSyntaxPosix);
-  EXPECT_STREQ("/foo/bar/.", fs_posix_trailing_slash.GetCString());
-  EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetDirectory().GetCString());
-  EXPECT_STREQ(".", fs_posix_trailing_slash.GetFilename().GetCString());
+  EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetCString());
+  EXPECT_STREQ("/foo", fs_posix_trailing_slash.GetDirectory().GetCString());
+  EXPECT_STREQ("bar", fs_posix_trailing_slash.GetFilename().GetCString());
 
   FileSpec fs_windows_trailing_slash("F:\\bar\\", false,
  FileSpec::ePathSyntaxWindows);
-  EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString());
-  // EXPECT_STREQ("F:\\bar",
-  // fs_windows_trailing_slash.GetDirectory().GetCString()); // It returns
-  // "F:/bar"
-  EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString());
+  EXPECT_STREQ("F:\\bar", fs_windows_trailing_slash.GetCString());
+  EXPECT_STREQ("bar", fs_windows_trailing_slash.GetFilename().GetCString());
 }
 
 TEST(FileSpecTest, AppendPathComponent) {
@@ -131,32 +128,13 @@
   EXPECT_STREQ("F:\\bar", fs_windows_root.GetCString());
 }
 
-static void Compare(const FileSpec &one, const FileSpec &two, bool full_match,
-bool remove_backup_dots, bool result) {
-  EXPECT_EQ(result, FileSpec::Equal(one, two, full_match, remove_backup_dots))
-  << "File one: " << one.GetCString() << "\nFile two: " << two.GetCString()
-  << "\nFull match: " << full_match
-  << "\nRemove backup dots: " << remove_backup_dots;
-}
-
 TEST(FileSpecTest, EqualSeparator) {
   FileSpec backward("C:\\foo\\bar", false, FileSpec::ePathSyntaxWindows);
   FileSpec forward("C:/foo/bar", false, FileSpec::ePathSyntaxWindows);
   EXPECT_EQ(forward, backward);
-
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
-  Compare(forward, backward, full_match, remove_backup_dots, match);
-  Compare(forward, backward, full_match, !remove_backup_dots, match);
-  Compare(forward, backward, !full_match, remove_backup_dots, match);
-  Compare(forward, backward, !full_match, !remove_backup_dots, match);
 }
 
 TEST(FileSpecTest, EqualDotsWindows) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair tests[] = {
   {R"(C:\foo\bar\baz)", R"(C:\foo\foo\..\bar\baz)"},
   {R"(C:\bar\baz)", R"(C:\foo\..\bar\baz)"},
@@ -170,18 +148,11 @@
   for (const auto &test : tests) {
 FileSpec one(test.first, false, FileSpec::ePathSyntaxWindows);
 FileSpec two(test.second, false, FileSpec::ePathSyntaxWindows);
-EXPECT_NE(one, two);
-Compare(one, two, full_match, remove_backup_dots, match);
-Compare(one, two, full_match, !remove_backup_dots, !match);
-Compare(one, two, !full_match, remove_backup_dots, match);
-Compare(one, two, !full_match, !remove_backup_dots, !match);
+EXPECT_EQ(one, two);
   }
 }
 
 TEST(FileSpecTest, EqualDotsPosix) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair tests[] = {
   {R"(/foo/bar/baz)", R"(/foo/foo/../bar/baz)"},
   {R"(/bar/baz)", R"(/foo/../bar/baz)"},
@@ -193,18 +164,11 @@
   for (const auto &test : tests) {
 FileSpec one(test.first, false, FileSpec::ePathSyntaxPosix);
 FileSpec two(test.second, false, FileSpec::ePathSyntaxPosix);
-EXPECT_NE(one, two);
-Compare(one, two, full_match, remove_backup_dots, match);
-Compare(one, two, full_match, !remove_backup_dots, !match);
-Compare(one, two, !full_match, remove_backup_dots, match);
-Compare(one, two, !full_match, !remove_backup_dots, !match);
+EXPECT_EQ(one, two);
   }
 }
 
 TEST(FileSpecTest, EqualDotsPosixRoot) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair tests[] = {
   {R"(/)", R"(/..)"},
   {R"(/)", R"(/.)"},
@@ -214,11 +178,7 @@
   for (const auto &test : tests) {
 FileSpec one(test.first, false, FileSpec

[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl added inline comments.



Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:189
+  // Always normalize our compile unit directory to get rid of redundant
+  // slashes and other path anonalies before we use it for path prepending
+  FileSpec local_spec(local_path, false);

`// slashes and other path ano*m*alies before we use it for path prepending*.*`



Comment at: source/Utility/FileSpec.cpp:105
+//--
+bool needsNormalization(const llvm::StringRef &path) {
+  for (auto i = path.find('/'); i != llvm::StringRef::npos;

Are we sure that we need this? Could we use llvm::sys::path::has_relative_path 
& friends instead? http://llvm.org/doxygen/namespacellvm_1_1sys_1_1path.html



Comment at: source/Utility/FileSpec.cpp:107
+  for (auto i = path.find('/'); i != llvm::StringRef::npos;
+   i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);

shouldn't this be a more general path separator (we probably have something 
like this in Support/Path.h)


https://reviews.llvm.org/D45977



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Greg Clayton via Phabricator via lldb-commits
clayborg added inline comments.



Comment at: source/Utility/FileSpec.cpp:105
+//--
+bool needsNormalization(const llvm::StringRef &path) {
+  for (auto i = path.find('/'); i != llvm::StringRef::npos;

I only want to scan the string one time. This will return true if this contains 
relative redundancies or extra slashes, not if the path is relative. "./foo" 
doesn't need normalization and this will return false, but "foo/." would need 
to be normalized to "foo", as well as "foo/./bar" and "foo/../bar". So this 
function is doing something completely different than the LLVM counterparts



Comment at: source/Utility/FileSpec.cpp:107
+  for (auto i = path.find('/'); i != llvm::StringRef::npos;
+   i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);

no as the only caller to this function switches the slashes to '/' already..


https://reviews.llvm.org/D45977



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Greg Clayton via Phabricator via lldb-commits
clayborg updated this revision to Diff 143643.
clayborg added a comment.

Fix comment typo


https://reviews.llvm.org/D45977

Files:
  include/lldb/Core/FileSpecList.h
  include/lldb/Utility/FileSpec.h
  source/Breakpoint/BreakpointResolverFileLine.cpp
  source/Core/FileSpecList.cpp
  source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
  source/Plugins/SymbolFile/DWARF/DWARFDebugLine.h
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  source/Symbol/CompileUnit.cpp
  source/Symbol/Declaration.cpp
  source/Utility/FileSpec.cpp
  unittests/Utility/FileSpecTest.cpp

Index: unittests/Utility/FileSpecTest.cpp
===
--- unittests/Utility/FileSpecTest.cpp
+++ unittests/Utility/FileSpecTest.cpp
@@ -54,17 +54,14 @@
 
   FileSpec fs_posix_trailing_slash("/foo/bar/", false,
FileSpec::ePathSyntaxPosix);
-  EXPECT_STREQ("/foo/bar/.", fs_posix_trailing_slash.GetCString());
-  EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetDirectory().GetCString());
-  EXPECT_STREQ(".", fs_posix_trailing_slash.GetFilename().GetCString());
+  EXPECT_STREQ("/foo/bar", fs_posix_trailing_slash.GetCString());
+  EXPECT_STREQ("/foo", fs_posix_trailing_slash.GetDirectory().GetCString());
+  EXPECT_STREQ("bar", fs_posix_trailing_slash.GetFilename().GetCString());
 
   FileSpec fs_windows_trailing_slash("F:\\bar\\", false,
  FileSpec::ePathSyntaxWindows);
-  EXPECT_STREQ("F:\\bar\\.", fs_windows_trailing_slash.GetCString());
-  // EXPECT_STREQ("F:\\bar",
-  // fs_windows_trailing_slash.GetDirectory().GetCString()); // It returns
-  // "F:/bar"
-  EXPECT_STREQ(".", fs_windows_trailing_slash.GetFilename().GetCString());
+  EXPECT_STREQ("F:\\bar", fs_windows_trailing_slash.GetCString());
+  EXPECT_STREQ("bar", fs_windows_trailing_slash.GetFilename().GetCString());
 }
 
 TEST(FileSpecTest, AppendPathComponent) {
@@ -131,32 +128,13 @@
   EXPECT_STREQ("F:\\bar", fs_windows_root.GetCString());
 }
 
-static void Compare(const FileSpec &one, const FileSpec &two, bool full_match,
-bool remove_backup_dots, bool result) {
-  EXPECT_EQ(result, FileSpec::Equal(one, two, full_match, remove_backup_dots))
-  << "File one: " << one.GetCString() << "\nFile two: " << two.GetCString()
-  << "\nFull match: " << full_match
-  << "\nRemove backup dots: " << remove_backup_dots;
-}
-
 TEST(FileSpecTest, EqualSeparator) {
   FileSpec backward("C:\\foo\\bar", false, FileSpec::ePathSyntaxWindows);
   FileSpec forward("C:/foo/bar", false, FileSpec::ePathSyntaxWindows);
   EXPECT_EQ(forward, backward);
-
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
-  Compare(forward, backward, full_match, remove_backup_dots, match);
-  Compare(forward, backward, full_match, !remove_backup_dots, match);
-  Compare(forward, backward, !full_match, remove_backup_dots, match);
-  Compare(forward, backward, !full_match, !remove_backup_dots, match);
 }
 
 TEST(FileSpecTest, EqualDotsWindows) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair tests[] = {
   {R"(C:\foo\bar\baz)", R"(C:\foo\foo\..\bar\baz)"},
   {R"(C:\bar\baz)", R"(C:\foo\..\bar\baz)"},
@@ -170,18 +148,11 @@
   for (const auto &test : tests) {
 FileSpec one(test.first, false, FileSpec::ePathSyntaxWindows);
 FileSpec two(test.second, false, FileSpec::ePathSyntaxWindows);
-EXPECT_NE(one, two);
-Compare(one, two, full_match, remove_backup_dots, match);
-Compare(one, two, full_match, !remove_backup_dots, !match);
-Compare(one, two, !full_match, remove_backup_dots, match);
-Compare(one, two, !full_match, !remove_backup_dots, !match);
+EXPECT_EQ(one, two);
   }
 }
 
 TEST(FileSpecTest, EqualDotsPosix) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair tests[] = {
   {R"(/foo/bar/baz)", R"(/foo/foo/../bar/baz)"},
   {R"(/bar/baz)", R"(/foo/../bar/baz)"},
@@ -193,18 +164,11 @@
   for (const auto &test : tests) {
 FileSpec one(test.first, false, FileSpec::ePathSyntaxPosix);
 FileSpec two(test.second, false, FileSpec::ePathSyntaxPosix);
-EXPECT_NE(one, two);
-Compare(one, two, full_match, remove_backup_dots, match);
-Compare(one, two, full_match, !remove_backup_dots, !match);
-Compare(one, two, !full_match, remove_backup_dots, match);
-Compare(one, two, !full_match, !remove_backup_dots, !match);
+EXPECT_EQ(one, two);
   }
 }
 
 TEST(FileSpecTest, EqualDotsPosixRoot) {
-  const bool full_match = true;
-  const bool remove_backup_dots = true;
-  const bool match = true;
   std::pair tests[] = {
   {R"(/)", R"(/..)"},
   {R"(/)", R"(/.)"},
@@ -214,11 +178,7 @@
   for (const auto &test : tests) {
 FileSpec one(test.first, false, FileSpec::ePathSyn

[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl added inline comments.



Comment at: source/Utility/FileSpec.cpp:107
+  for (auto i = path.find('/'); i != llvm::StringRef::npos;
+   i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);

clayborg wrote:
> no as the only caller to this function switches the slashes to '/' already..
If we change that to also replace double-`/` with singles, we could replace 
this function with a call to llvm::sys::path::remove_dots()

http://llvm.org/doxygen/namespacellvm_1_1sys_1_1path.html#a35c103b5fb70a66a1cb5da3b56f588a1



Comment at: source/Utility/FileSpec.cpp:115
+  case '/':
+// "//" in the middle of a path needs to be normalized
+if (i > 0)

Does this also turn "//WORKGROUP/Foo" into "/WORKGROUP/Foo/"?


https://reviews.llvm.org/D45977



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl added a comment.

One general question: why is this form of normalization preferred over calling 
realpath?


https://reviews.llvm.org/D45977



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Greg Clayton via Phabricator via lldb-commits
clayborg added inline comments.



Comment at: source/Utility/FileSpec.cpp:107
+  for (auto i = path.find('/'); i != llvm::StringRef::npos;
+   i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);

I am fine switching to using the llvm functions for removing, this is only 
detecting if any normalization needs to happen. If there is an equivalent LLVM 
function that will only run through the string one time to detect all needs for 
normalization (no multiple passes looking for "..", then for "." etc like we 
used to have), I would be happy to use it, but there doesn't seem to be. We are 
trying to avoid having to create a "SmallVectorImpl< char >" for all paths if 
they don't need fixing here. This function is just iterating through an 
llvm::StringRef just to see if normalization needs to happen. If it does, then 
we make a SmallVectorImpl< char > and we fix the path. We can easily use llvm 
functions for the fixing part. 



Comment at: source/Utility/FileSpec.cpp:115
+  case '/':
+// "//" in the middle of a path needs to be normalized
+if (i > 0)

aprantl wrote:
> Does this also turn "//WORKGROUP/Foo" into "/WORKGROUP/Foo/"?
No it does not,. Notice the "if (i > 0)" below. We need to keep leading "//"


https://reviews.llvm.org/D45977



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Greg Clayton via Phabricator via lldb-commits
clayborg added a comment.

In https://reviews.llvm.org/D45977#1076048, @aprantl wrote:

> One general question: why is this form of normalization preferred over 
> calling realpath?


Normalization is everything we can do to fix up a path without knowing anything 
about the current working directory or any symlinks. So we can remove redundant 
references to the current directory (".") or the parent directory (".."), but 
only if they are not at the start of the path. Since our path may have been 
created on a different machine with an unknown symlinks, realpath really can't 
do anything for us. What is a path like "./foo.txt" relative to when stored in 
the debug info with no compilation directory? what if we have "/tmp/mysymlink" 
mean on another machine? We can't realpath it because we don't know the actual 
root or any of the symlinks.


https://reviews.llvm.org/D45977



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl added inline comments.



Comment at: source/Utility/FileSpec.cpp:107
+  for (auto i = path.find('/'); i != llvm::StringRef::npos;
+   i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);

clayborg wrote:
> I am fine switching to using the llvm functions for removing, this is only 
> detecting if any normalization needs to happen. If there is an equivalent 
> LLVM function that will only run through the string one time to detect all 
> needs for normalization (no multiple passes looking for "..", then for "." 
> etc like we used to have), I would be happy to use it, but there doesn't seem 
> to be. We are trying to avoid having to create a "SmallVectorImpl< char >" 
> for all paths if they don't need fixing here. This function is just iterating 
> through an llvm::StringRef just to see if normalization needs to happen. If 
> it does, then we make a SmallVectorImpl< char > and we fix the path. We can 
> easily use llvm functions for the fixing part. 
I see, you want to avoid copying the string when it isn't necessary to do so. 
IMHO the best way to do this is to add a `bool hasDots(const Twine &)` function 
to llvm::sys::path and add a `bool remove_dot_dot` parameter to `removeDots()`. 
Since you have already written the unittests, adding the function to LLVM 
shouldn't be any extra work (I'll help reviewing), and we'll have 100 LoC less 
to maintain.

This way we can avoid having two functions that perform path normalization that 
superficially look like they might do the same thing, but a year from now 
nobody can really tell whether they behave exactly the same, and whether a bug 
found in one implementation should also be fixed in the other one, etc... .


https://reviews.llvm.org/D45977



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Greg Clayton via Phabricator via lldb-commits
clayborg added inline comments.



Comment at: source/Utility/FileSpec.cpp:107
+  for (auto i = path.find('/'); i != llvm::StringRef::npos;
+   i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);

aprantl wrote:
> clayborg wrote:
> > I am fine switching to using the llvm functions for removing, this is only 
> > detecting if any normalization needs to happen. If there is an equivalent 
> > LLVM function that will only run through the string one time to detect all 
> > needs for normalization (no multiple passes looking for "..", then for "." 
> > etc like we used to have), I would be happy to use it, but there doesn't 
> > seem to be. We are trying to avoid having to create a "SmallVectorImpl< 
> > char >" for all paths if they don't need fixing here. This function is just 
> > iterating through an llvm::StringRef just to see if normalization needs to 
> > happen. If it does, then we make a SmallVectorImpl< char > and we fix the 
> > path. We can easily use llvm functions for the fixing part. 
> I see, you want to avoid copying the string when it isn't necessary to do so. 
> IMHO the best way to do this is to add a `bool hasDots(const Twine &)` 
> function to llvm::sys::path and add a `bool remove_dot_dot` parameter to 
> `removeDots()`. Since you have already written the unittests, adding the 
> function to LLVM shouldn't be any extra work (I'll help reviewing), and we'll 
> have 100 LoC less to maintain.
> 
> This way we can avoid having two functions that perform path normalization 
> that superficially look like they might do the same thing, but a year from 
> now nobody can really tell whether they behave exactly the same, and whether 
> a bug found in one implementation should also be fixed in the other one, 
> etc... .
I want to know if there are redundant slashes as well. I want something like 
"bool llvm::sys::has(const Twine &, bool dots, bool dot_dots, bool 
redundant_slashes)". But I don't see that getting accepted? I just want a "bool 
llvm:sys::path::contains_redundant_stuff(const Twine &)".  Not sure on the name 
though. needs_normalization? can_be_shortened? Everything in LLVM right now 
assumes that dots, dot dots, slashes and realpath stuff happen in the same 
function. Not sure how to break that up without ruining the performance of the 
one and only loop that should be happening. I like the current approach since 
it doesn't require chopping up the string into an array and iterating through 
the array.


https://reviews.llvm.org/D45977



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Greg Clayton via Phabricator via lldb-commits
clayborg added inline comments.



Comment at: source/Utility/FileSpec.cpp:107
+  for (auto i = path.find('/'); i != llvm::StringRef::npos;
+   i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);

clayborg wrote:
> aprantl wrote:
> > clayborg wrote:
> > > I am fine switching to using the llvm functions for removing, this is 
> > > only detecting if any normalization needs to happen. If there is an 
> > > equivalent LLVM function that will only run through the string one time 
> > > to detect all needs for normalization (no multiple passes looking for 
> > > "..", then for "." etc like we used to have), I would be happy to use it, 
> > > but there doesn't seem to be. We are trying to avoid having to create a 
> > > "SmallVectorImpl< char >" for all paths if they don't need fixing here. 
> > > This function is just iterating through an llvm::StringRef just to see if 
> > > normalization needs to happen. If it does, then we make a 
> > > SmallVectorImpl< char > and we fix the path. We can easily use llvm 
> > > functions for the fixing part. 
> > I see, you want to avoid copying the string when it isn't necessary to do 
> > so. IMHO the best way to do this is to add a `bool hasDots(const Twine &)` 
> > function to llvm::sys::path and add a `bool remove_dot_dot` parameter to 
> > `removeDots()`. Since you have already written the unittests, adding the 
> > function to LLVM shouldn't be any extra work (I'll help reviewing), and 
> > we'll have 100 LoC less to maintain.
> > 
> > This way we can avoid having two functions that perform path normalization 
> > that superficially look like they might do the same thing, but a year from 
> > now nobody can really tell whether they behave exactly the same, and 
> > whether a bug found in one implementation should also be fixed in the other 
> > one, etc... .
> I want to know if there are redundant slashes as well. I want something like 
> "bool llvm::sys::has(const Twine &, bool dots, bool dot_dots, bool 
> redundant_slashes)". But I don't see that getting accepted? I just want a 
> "bool llvm:sys::path::contains_redundant_stuff(const Twine &)".  Not sure on 
> the name though. needs_normalization? can_be_shortened? Everything in LLVM 
> right now assumes that dots, dot dots, slashes and realpath stuff happen in 
> the same function. Not sure how to break that up without ruining the 
> performance of the one and only loop that should be happening. I like the 
> current approach since it doesn't require chopping up the string into an 
> array and iterating through the array.
Also not sure if the LLVM stuff will try to substitute in the current working 
directory for "."?


https://reviews.llvm.org/D45977



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl added inline comments.



Comment at: source/Utility/FileSpec.cpp:107
+  for (auto i = path.find('/'); i != llvm::StringRef::npos;
+   i = path.find('/', i + 1)) {
+const auto nextChar = safeCharAtIndex(path, i+1);

clayborg wrote:
> clayborg wrote:
> > aprantl wrote:
> > > clayborg wrote:
> > > > I am fine switching to using the llvm functions for removing, this is 
> > > > only detecting if any normalization needs to happen. If there is an 
> > > > equivalent LLVM function that will only run through the string one time 
> > > > to detect all needs for normalization (no multiple passes looking for 
> > > > "..", then for "." etc like we used to have), I would be happy to use 
> > > > it, but there doesn't seem to be. We are trying to avoid having to 
> > > > create a "SmallVectorImpl< char >" for all paths if they don't need 
> > > > fixing here. This function is just iterating through an llvm::StringRef 
> > > > just to see if normalization needs to happen. If it does, then we make 
> > > > a SmallVectorImpl< char > and we fix the path. We can easily use llvm 
> > > > functions for the fixing part. 
> > > I see, you want to avoid copying the string when it isn't necessary to do 
> > > so. IMHO the best way to do this is to add a `bool hasDots(const Twine 
> > > &)` function to llvm::sys::path and add a `bool remove_dot_dot` parameter 
> > > to `removeDots()`. Since you have already written the unittests, adding 
> > > the function to LLVM shouldn't be any extra work (I'll help reviewing), 
> > > and we'll have 100 LoC less to maintain.
> > > 
> > > This way we can avoid having two functions that perform path 
> > > normalization that superficially look like they might do the same thing, 
> > > but a year from now nobody can really tell whether they behave exactly 
> > > the same, and whether a bug found in one implementation should also be 
> > > fixed in the other one, etc... .
> > I want to know if there are redundant slashes as well. I want something 
> > like "bool llvm::sys::has(const Twine &, bool dots, bool dot_dots, bool 
> > redundant_slashes)". But I don't see that getting accepted? I just want a 
> > "bool llvm:sys::path::contains_redundant_stuff(const Twine &)".  Not sure 
> > on the name though. needs_normalization? can_be_shortened? Everything in 
> > LLVM right now assumes that dots, dot dots, slashes and realpath stuff 
> > happen in the same function. Not sure how to break that up without ruining 
> > the performance of the one and only loop that should be happening. I like 
> > the current approach since it doesn't require chopping up the string into 
> > an array and iterating through the array.
> Also not sure if the LLVM stuff will try to substitute in the current working 
> directory for "."?
I just read the source code of remove_dots() 
(http://llvm.org/doxygen/Path_8cpp_source.html#l00699) and if I'm not mistaken, 
it actually also removes double-separators as a side-effect, since it iterates 
over the path components of the string as it constructs the copy. It also seems 
to avoid the copy operation if it isn't necessary.

Could you take another look? Perhaps I'm missing something, (or perhaps we can 
just turn this into a small addition to remove_dots).

> Everything in LLVM right now assumes that dots, dot dots, slashes and 
> realpath stuff happen in the same function.
I'm not sure I understand.


https://reviews.llvm.org/D45977



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl added a comment.

> Also not sure if the LLVM stuff will try to substitute in the current working 
> directory for "."?

No it won't, remove_dots() has no side effects. There's a separate 
make_absolute() function. That one will cause another copy. Personally, if I 
have a choice between a redundant string copy and maintaining more code, I'd 
pick the string copy unless we know that it really matters.


https://reviews.llvm.org/D45977



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D45977: Always normalize FileSpec paths.

2018-04-23 Thread Adrian Prantl via Phabricator via lldb-commits
aprantl added a comment.

I'll take that back: make_absolute only copies when the path isn't already 
absolute.


https://reviews.llvm.org/D45977



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits