This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] PR libstdc++/71044 optimize std::filesystem::path construction
On 13/12/18 20:34 +0000, Jonathan Wakely wrote:
Construction and modification of paths is now done more efficiently, by
splitting the input into a stack-based buffer of string_view objects
instead of a dynamically-allocated vector containing strings. Once the
final size is known only a single allocation is needed to reserve space
for it. The append and concat operations no longer require constructing
temporary path objects, nor re-parsing the entire native pathname.
This patch fixes a regression in the append/concat code, introduced by
the changes to avoid constructing an intermediate path. The problem
wasn't caught by existing tests, so I've improved the tests.
Tested x86_64-linux, committed to trunk.
commit 7583088c09b5dd5d33a98abd1f61b0ccff961d6e
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Mon Dec 17 21:42:48 2018 +0000
PR libstdc++/71044 fix off-by-one errors introduced recently
The recent changes to append/concat directly from strings (without
constructing paths) introduced regressions where one of the components
could be omitted from the iteration sequence in the result.
PR libstdc++/71044
* src/filesystem/std-path.cc (path::_M_append): Fix off-by-one error
that caused a component to be lost from the iteration sequence.
(path::_M_concat): Likewise.
* testsuite/27_io/filesystem/path/append/source.cc: Test appending
long strings.
* testsuite/27_io/filesystem/path/concat/strings.cc: Test
concatenating long strings.
* testsuite/27_io/filesystem/path/construct/string_view.cc: Test
construction from long string.
diff --git a/libstdc++-v3/src/filesystem/std-path.cc b/libstdc++-v3/src/filesystem/std-path.cc
index d2520492c03..b5ddbdad149 100644
--- a/libstdc++-v3/src/filesystem/std-path.cc
+++ b/libstdc++-v3/src/filesystem/std-path.cc
@@ -781,10 +781,11 @@ path::_M_append(basic_string_view<value_type> s)
::new(output++) _Cmpt(c.str, c.type, parser.offset(c));
++_M_cmpts._M_impl->_M_size;
}
- for (auto c = parser.next(); c.valid(); c = parser.next())
+ while (cmpt.valid())
{
- ::new(output++) _Cmpt(c.str, c.type, parser.offset(c));
+ ::new(output++) _Cmpt(cmpt.str, cmpt.type, parser.offset(cmpt));
++_M_cmpts._M_impl->_M_size;
+ cmpt = parser.next();
}
if (s.back() == '/')
@@ -1139,7 +1140,7 @@ path::_M_concat(basic_string_view<value_type> s)
}
#endif
}
- else if (orig_filenamelen == 0)
+ else if (orig_filenamelen == 0 && extra.empty())
{
// Replace empty filename at end of original path.
std::prev(output)->_M_pathname = it->str;
diff --git a/libstdc++-v3/testsuite/27_io/filesystem/path/append/source.cc b/libstdc++-v3/testsuite/27_io/filesystem/path/append/source.cc
index e440ca921c7..21ae6be3d97 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/path/append/source.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/path/append/source.cc
@@ -137,6 +137,22 @@ test05()
s = second->native();
p3 /= s;
VERIFY( p3.string() == "0/123456789/a/123456789" );
+ }
+
+void
+test06()
+{
+ const std::string s0 = "a/b/c";
+ path p = s0;
+ std::string s;
+ for (int i = 0; i < 10; ++i)
+ s += "0/1/2/3/4/5/6/7/8/9/";
+ // append a long string with many components
+ test(p, s.c_str());
+
+ // Same again but with a trailing slash on the left operand:
+ path p2 = s0 + '/';
+ test(p2, s.c_str());
}
int
@@ -147,4 +163,5 @@ main()
test03();
test04();
test05();
+ test06();
}
diff --git a/libstdc++-v3/testsuite/27_io/filesystem/path/concat/strings.cc b/libstdc++-v3/testsuite/27_io/filesystem/path/concat/strings.cc
index eea9b6dc69b..316f7c3d7bd 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/path/concat/strings.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/path/concat/strings.cc
@@ -24,8 +24,10 @@
#include <filesystem>
#include <testsuite_hooks.h>
#include <testsuite_iterators.h>
+#include <testsuite_fs.h>
using std::filesystem::path;
+using __gnu_test::compare_paths;
void
test01()
@@ -60,7 +62,7 @@ test01()
void
test02()
{
- std::basic_string_view<path::value_type> s;
+ std::basic_string_view<path::value_type> s, expected;
path p = "0/1/2/3/4/5/6";
// The string_view aliases the path's internal string:
@@ -68,25 +70,54 @@ test02()
// Append that string_view, which must work correctly even though the
// internal string will be reallocated during the operation:
p += s;
- VERIFY( p.string() == "0/1/2/3/4/5/60/1/2/3/4/5/6" );
+ compare_paths(p, "0/1/2/3/4/5/60/1/2/3/4/5/6");
// Same again with a trailing slash:
path p2 = "0/1/2/3/4/5/";
s = p2.native();
p2 += s;
- VERIFY( p2.string() == "0/1/2/3/4/5/0/1/2/3/4/5/" );
+ compare_paths(p2, "0/1/2/3/4/5/0/1/2/3/4/5/");
// And aliasing one of the components of the path:
path p3 = "0/123456789";
path::iterator second = std::next(p3.begin());
s = second->native();
p3 += s;
- VERIFY( p3.string() == "0/123456789123456789" );
+ compare_paths(p3, "0/123456789123456789" );
}
+void
+test03()
+{
+ const std::string s0 = "a/b/c";
+ path p = s0;
+ std::string s;
+ for (int i = 0; i < 10; ++i)
+ s += "0/1/2/3/4/5/6/7/8/9/";
+ // concat a long string with many components:
+ p += s;
+ compare_paths(p, path(s0+s));
+
+ // Same again but with a trailing slash on the left operand:
+ path p2 = s0 + '/';
+ p2 += s;
+ compare_paths(p2, path(s0+'/'+s));
+
+ // And again but with a leading slash on the right operand:
+ path p3 = s0;
+ s.insert(0, 1, '/');
+ p3 += s;
+ compare_paths(p2, path(s0+s));
+
+ // And again but with a slash on both operands:
+ path p4 = s0 + '/';
+ p4 += s;
+ compare_paths(p4, path(s0+'/'+s));
+}
int
main()
{
test01();
test02();
+ test03();
}
diff --git a/libstdc++-v3/testsuite/27_io/filesystem/path/construct/string_view.cc b/libstdc++-v3/testsuite/27_io/filesystem/path/construct/string_view.cc
index 0339df73b30..a6130d389bf 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/path/construct/string_view.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/path/construct/string_view.cc
@@ -25,6 +25,7 @@
#include <string_view>
#include <string>
#include <testsuite_fs.h>
+#include <testsuite_hooks.h>
using std::filesystem::path;
using __gnu_test::compare_paths;
@@ -49,8 +50,31 @@ test01()
}
}
+void
+test02()
+{
+ std::string s;
+ for (int i = 0; i < 10; ++i)
+ s += "0/1/2/3/4/5/6/7/8/9/";
+ // Construct a path with a large number of components:
+ path p = s;
+ auto iter = p.begin();
+ for (int i = 0; i < 100; ++i)
+ {
+ char c = '0' + i % 10;
+ std::string_view sv(&c, 1);
+ VERIFY( iter != p.end() );
+ compare_paths( *iter, sv );
+ ++iter;
+ }
+ VERIFY( iter != p.end() );
+ VERIFY( iter->native().empty() );
+ VERIFY( ++iter == p.end() );
+}
+
int
main()
{
test01();
+ test02();
}