From patchwork Wed Nov 28 15:27:04 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 1004594 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-491110-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="nQ2Po/g7"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 434kzr2KRLz9ryk for ; Thu, 29 Nov 2018 02:27:35 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; q=dns; s= default; b=WcU+05SBNllLfAST/Tqoqo07g0ZUW9CR40GAGWja4YljDqiyg+Gca Em4e4XzkZvl0BxwEza6JbDHt5lezXF1Eh0B3AICAkNpBx1SGv6dJwet9WnDPQdVV QpyBKqiU3DL1TX+HoaB3TBu4oOWQ/aO9JscMxbWIsPQB1/P9dmhduU= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; s= default; bh=sYXtIBnv+umdmzNt4XVYUDbQpf4=; b=nQ2Po/g7qaNZovHDWDmk mJgOK3DWFhhQFHMZI7j6NQ/hPWQQb9B2hrtA4EdlI3/P3WRn04cAFd1wQavPwazV JrAJc9LX+NSYXmFTjxFqY/8uAtHF8S4zcuqaFOTsO6ZsQlKkArhJ4h7HNrV8niPL XZ+nchxYOp6g6PDcB3RnJoA= Received: (qmail 57739 invoked by alias); 28 Nov 2018 15:27:18 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 57704 invoked by uid 89); 28 Nov 2018 15:27:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, KAM_SHORT, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=11598 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 28 Nov 2018 15:27:08 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D091C309BD24; Wed, 28 Nov 2018 15:27:06 +0000 (UTC) Received: from localhost (unknown [10.33.36.64]) by smtp.corp.redhat.com (Postfix) with ESMTP id 828796C21C; Wed, 28 Nov 2018 15:27:05 +0000 (UTC) Date: Wed, 28 Nov 2018 15:27:04 +0000 From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [PATCH] PR libstdc++/83306 make filesystem_error no-throw copyable Message-ID: <20181128152704.GA24994@redhat.com> MIME-Version: 1.0 Content-Disposition: inline X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.9.2 (2017-12-15) The class API provides no way to modify the members, so we can share them between copies of the same object. Copying becomes a simple reference count update, which doesn't throw. Also adjust the what() string to allow distinguishing between an empty path passed to the constructor, and no path. PR libstdc++/83306 * include/bits/fs_path.h (filesystem_error): Move data members into pimpl class owned by shared_ptr. Remove inline definitions of member functions. * src/filesystem/std-path.cc (filesystem_error::_Impl): Define. (filesystem_error): Define member functions. * testsuite/27_io/filesystem/filesystem_error/cons.cc: New test. * testsuite/27_io/filesystem/filesystem_error/copy.cc: New test. Tested x86_64-linux, committed to trunk. commit 45dcc3724dea7e22e5baa32fc8e18c024a649fba Author: Jonathan Wakely Date: Wed Nov 28 14:46:40 2018 +0000 PR libstdc++/83306 make filesystem_error no-throw copyable The class API provides no way to modify the members, so we can share them between copies of the same object. Copying becomes a simple reference count update, which doesn't throw. Also adjust the what() string to allow distinguishing between an empty path passed to the constructor, and no path. PR libstdc++/83306 * include/bits/fs_path.h (filesystem_error): Move data members into pimpl class owned by shared_ptr. Remove inline definitions of member functions. * src/filesystem/std-path.cc (filesystem_error::_Impl): Define. (filesystem_error): Define member functions. * testsuite/27_io/filesystem/filesystem_error/cons.cc: New test. * testsuite/27_io/filesystem/filesystem_error/copy.cc: New test. diff --git a/libstdc++-v3/include/bits/fs_path.h b/libstdc++-v3/include/bits/fs_path.h index e3938d06d59..0eee684a2f6 100644 --- a/libstdc++-v3/include/bits/fs_path.h +++ b/libstdc++-v3/include/bits/fs_path.h @@ -43,6 +43,8 @@ #include #include #include +#include +#include #if defined(_WIN32) && !defined(__CYGWIN__) # define _GLIBCXX_FILESYSTEM_IS_WINDOWS 1 @@ -575,30 +577,29 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 class filesystem_error : public std::system_error { public: - filesystem_error(const string& __what_arg, error_code __ec) - : system_error(__ec, __what_arg) { } + filesystem_error(const string& __what_arg, error_code __ec); filesystem_error(const string& __what_arg, const path& __p1, - error_code __ec) - : system_error(__ec, __what_arg), _M_path1(__p1) { } + error_code __ec); filesystem_error(const string& __what_arg, const path& __p1, - const path& __p2, error_code __ec) - : system_error(__ec, __what_arg), _M_path1(__p1), _M_path2(__p2) - { } + const path& __p2, error_code __ec); + + filesystem_error(const filesystem_error&) = default; + filesystem_error& operator=(const filesystem_error&) = default; + + // No move constructor or assignment operator. + // Copy rvalues instead, so that _M_impl is not left empty. ~filesystem_error(); - const path& path1() const noexcept { return _M_path1; } - const path& path2() const noexcept { return _M_path2; } - const char* what() const noexcept { return _M_what.c_str(); } + const path& path1() const noexcept; + const path& path2() const noexcept; + const char* what() const noexcept; private: - std::string _M_gen_what(); - - path _M_path1; - path _M_path2; - std::string _M_what = _M_gen_what(); + struct _Impl; + std::__shared_ptr _M_impl; }; struct path::_Cmpt : path @@ -1158,6 +1159,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 _GLIBCXX_END_NAMESPACE_CXX11 } // namespace filesystem +extern template class __shared_ptr; + _GLIBCXX_END_NAMESPACE_VERSION } // namespace std diff --git a/libstdc++-v3/src/filesystem/std-path.cc b/libstdc++-v3/src/filesystem/std-path.cc index f382eb3759a..fb8898d9040 100644 --- a/libstdc++-v3/src/filesystem/std-path.cc +++ b/libstdc++-v3/src/filesystem/std-path.cc @@ -34,8 +34,6 @@ namespace fs = std::filesystem; using fs::path; -fs::filesystem_error::~filesystem_error() = default; - constexpr path::value_type path::preferred_separator; #ifdef _GLIBCXX_FILESYSTEM_IS_WINDOWS @@ -741,47 +739,83 @@ fs::hash_value(const path& p) noexcept return seed; } -namespace std +struct fs::filesystem_error::_Impl { -_GLIBCXX_BEGIN_NAMESPACE_VERSION -namespace filesystem -{ - string - fs_err_concat(const string& __what, const string& __path1, - const string& __path2) + _Impl(const string& what_arg, const path& p1, const path& p2) + : path1(p1), path2(p2), what(make_what(what_arg, &p1, &p2)) + { } + + _Impl(const string& what_arg, const path& p1) + : path1(p1), path2(), what(make_what(what_arg, &p1, nullptr)) + { } + + _Impl(const string& what_arg) + : what(make_what(what_arg, nullptr, nullptr)) + { } + + static std::string + make_what(const std::string& s, const path* p1, const path* p2) { - const size_t __len = 18 + __what.length() - + (__path1.length() ? __path1.length() + 3 : 0) - + (__path2.length() ? __path2.length() + 3 : 0); - string __ret; - __ret.reserve(__len); - __ret = "filesystem error: "; - __ret += __what; - if (!__path1.empty()) + const std::string pstr1 = p1 ? p1->u8string() : std::string{}; + const std::string pstr2 = p2 ? p2->u8string() : std::string{}; + const size_t len = 18 + s.length() + + (pstr1.length() ? pstr1.length() + 3 : 0) + + (pstr2.length() ? pstr2.length() + 3 : 0); + std::string w; + w.reserve(len); + w = "filesystem error: "; + w += s; + if (p1) { - __ret += " ["; - __ret += __path1; - __ret += ']'; + w += " ["; + w += pstr1; + w += ']'; + if (p2) + { + w += " ["; + w += pstr2; + w += ']'; + } } - if (!__path2.empty()) - { - __ret += " ["; - __ret += __path2; - __ret += ']'; - } - return __ret; + return w; } -_GLIBCXX_BEGIN_NAMESPACE_CXX11 + path path1; + path path2; + std::string what; +}; - std::string filesystem_error::_M_gen_what() - { - return fs_err_concat(system_error::what(), _M_path1.u8string(), - _M_path2.u8string()); - } +template class std::__shared_ptr; -_GLIBCXX_END_NAMESPACE_CXX11 +fs::filesystem_error:: +filesystem_error(const string& what_arg, error_code ec) +: system_error(ec, what_arg), + _M_impl(std::__make_shared<_Impl>(what_arg)) +{ } -} // filesystem -_GLIBCXX_END_NAMESPACE_VERSION -} // std +fs::filesystem_error:: +filesystem_error(const string& what_arg, const path& p1, error_code ec) +: system_error(ec, what_arg), + _M_impl(std::__make_shared<_Impl>(what_arg, p1)) +{ } + +fs::filesystem_error:: +filesystem_error(const string& what_arg, const path& p1, const path& p2, + error_code ec) +: system_error(ec, what_arg), + _M_impl(std::__make_shared<_Impl>(what_arg, p1, p2)) +{ } + +fs::filesystem_error::~filesystem_error() = default; + +const fs::path& +fs::filesystem_error::path1() const noexcept +{ return _M_impl->path1; } + +const fs::path& +fs::filesystem_error::path2() const noexcept +{ return _M_impl->path2; } + +const char* +fs::filesystem_error::what() const noexcept +{ return _M_impl->what.c_str(); } diff --git a/libstdc++-v3/testsuite/27_io/filesystem/filesystem_error/cons.cc b/libstdc++-v3/testsuite/27_io/filesystem/filesystem_error/cons.cc new file mode 100644 index 00000000000..ddaaf44d1a5 --- /dev/null +++ b/libstdc++-v3/testsuite/27_io/filesystem/filesystem_error/cons.cc @@ -0,0 +1,93 @@ +// Copyright (C) 2018 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// . + +// { dg-options "-std=gnu++17 -lstdc++fs" } +// { dg-do run { target c++17 } } +// { dg-require-filesystem-ts "" } + +#include +#include + +using std::filesystem::filesystem_error; + +bool contains(std::string_view what_str, std::string_view expected) +{ + return what_str.find(expected) != std::string_view::npos; +} + +void +test01() +{ + const char* const str = "error test"; + const std::error_code ec = make_error_code(std::errc::is_a_directory); + const std::filesystem::path p1 = "test/path/one"; + const std::filesystem::path p2 = "/test/path/two"; + + const filesystem_error e1(str, ec); + VERIFY( contains(e1.what(), str) ); + VERIFY( !contains(e1.what(), "[]") ); // no "empty path" in the string + VERIFY( e1.path1().empty() ); + VERIFY( e1.path2().empty() ); + VERIFY( e1.code() == ec ); + + const filesystem_error e2(str, p1, ec); + VERIFY( e2.path1() == p1 ); + VERIFY( e2.path2().empty() ); + VERIFY( contains(e2.what(), str) ); + VERIFY( contains(e2.what(), p1.string()) ); + VERIFY( !contains(e2.what(), "[]") ); + VERIFY( e2.code() == ec ); + + const filesystem_error e3(str, std::filesystem::path{}, ec); + VERIFY( e3.path1().empty() ); + VERIFY( e3.path2().empty() ); + VERIFY( contains(e3.what(), str) ); + VERIFY( contains(e3.what(), "[]") ); + VERIFY( !contains(e3.what(), "[] []") ); + VERIFY( e3.code() == ec ); + + const filesystem_error e4(str, p1, p2, ec); + VERIFY( e4.path1() == p1 ); + VERIFY( e4.path2() == p2 ); + VERIFY( contains(e4.what(), str) ); + VERIFY( contains(e4.what(), p1.string()) ); + VERIFY( contains(e4.what(), p2.string()) ); + VERIFY( !contains(e4.what(), "[]") ); + VERIFY( e4.code() == ec ); + + const filesystem_error e5(str, p1, std::filesystem::path{}, ec); + VERIFY( e5.path1() == p1 ); + VERIFY( e5.path2().empty() ); + VERIFY( contains(e5.what(), str) ); + VERIFY( contains(e5.what(), p1.string()) ); + VERIFY( contains(e5.what(), "[]") ); + VERIFY( e5.code() == ec ); + + const filesystem_error e6(str, std::filesystem::path{}, p2, ec); + VERIFY( e6.path1().empty() ); + VERIFY( e6.path2() == p2 ); + VERIFY( contains(e6.what(), str) ); + VERIFY( contains(e6.what(), "[]") ); + VERIFY( contains(e6.what(), p2.string()) ); + VERIFY( e6.code() == ec ); +} + +int +main() +{ + test01(); +} diff --git a/libstdc++-v3/testsuite/27_io/filesystem/filesystem_error/copy.cc b/libstdc++-v3/testsuite/27_io/filesystem/filesystem_error/copy.cc new file mode 100644 index 00000000000..f085f9d6794 --- /dev/null +++ b/libstdc++-v3/testsuite/27_io/filesystem/filesystem_error/copy.cc @@ -0,0 +1,111 @@ +// Copyright (C) 2018 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// . + +// { dg-options "-std=gnu++17 -lstdc++fs" } +// { dg-do run { target c++17 } } +// { dg-require-filesystem-ts "" } + +#include +#include + +using std::filesystem::filesystem_error; + +// PR libstdc++/83306 +static_assert(std::is_nothrow_copy_constructible_v); +static_assert(std::is_nothrow_copy_assignable_v); + +void +test01() +{ + const char* const str = "error test"; + const std::error_code ec = make_error_code(std::errc::is_a_directory); + const filesystem_error e1(str, ec); + auto e2 = e1; + VERIFY( e2.path1().empty() ); + VERIFY( e2.path2().empty() ); + VERIFY( std::string_view(e2.what()).find(str) != std::string_view::npos ); + VERIFY( e2.code() == ec ); + + const filesystem_error e3(str, "test/path/one", ec); + auto e4 = e3; + VERIFY( e4.path1() == "test/path/one" ); + VERIFY( e4.path2().empty() ); + VERIFY( std::string_view(e4.what()).find(str) != std::string_view::npos ); + VERIFY( e2.code() == ec ); + + const filesystem_error e5(str, "test/path/one", "/test/path/two", ec); + auto e6 = e5; + VERIFY( e6.path1() == "test/path/one" ); + VERIFY( e6.path2() == "/test/path/two" ); + VERIFY( std::string_view(e6.what()).find(str) != std::string_view::npos ); + VERIFY( e2.code() == ec ); +} + +void +test02() +{ + const char* const str = "error test"; + const std::error_code ec = make_error_code(std::errc::is_a_directory); + const filesystem_error e1(str, ec); + filesystem_error e2("", {}); + e2 = e1; + VERIFY( e2.path1().empty() ); + VERIFY( e2.path2().empty() ); + VERIFY( std::string_view(e2.what()).find(str) != std::string_view::npos ); + VERIFY( e2.code() == ec ); + + const filesystem_error e3(str, "test/path/one", ec); + filesystem_error e4("", {}); + e4 = e3; + VERIFY( e4.path1() == "test/path/one" ); + VERIFY( e4.path2().empty() ); + VERIFY( std::string_view(e4.what()).find(str) != std::string_view::npos ); + VERIFY( e2.code() == ec ); + + const filesystem_error e5(str, "test/path/one", "/test/path/two", ec); + filesystem_error e6("", {}); + e6 = e5; + VERIFY( e6.path1() == "test/path/one" ); + VERIFY( e6.path2() == "/test/path/two" ); + VERIFY( std::string_view(e6.what()).find(str) != std::string_view::npos ); + VERIFY( e2.code() == ec ); +} + +void +test03() +{ + filesystem_error e("test", std::error_code()); + VERIFY( e.path1().empty() ); + VERIFY( e.path2().empty() ); + auto e2 = std::move(e); + // Observers must still be usable on moved-from object: + VERIFY( e.path1().empty() ); + VERIFY( e.path2().empty() ); + VERIFY( e.what() != nullptr ); + e2 = std::move(e); + VERIFY( e.path1().empty() ); + VERIFY( e.path2().empty() ); + VERIFY( e.what() != nullptr ); +} + +int +main() +{ + test01(); + test02(); + test03(); +}