From c382d1977d6035628627c4ed132140c697882bbd Mon Sep 17 00:00:00 2001 From: Herb Sutter Date: Fri, 11 Aug 2023 09:36:09 -1000 Subject: [PATCH] Reject chained assignment, don't emit `[[nodiscard]]` on = and ?= (See #368 comments) --- .../pure2-bugfix-for-discard-precedence.cpp2 | 4 +- .../pure2-bugfix-for-discard-precedence.cpp | 8 +-- regression-tests/test-results/version | 2 +- source/build.info | 2 +- source/cppfront.cpp | 2 + source/parse.h | 64 ++++++++++++++++++- 6 files changed, 72 insertions(+), 10 deletions(-) diff --git a/regression-tests/pure2-bugfix-for-discard-precedence.cpp2 b/regression-tests/pure2-bugfix-for-discard-precedence.cpp2 index f6928a9783..2a30653a49 100644 --- a/regression-tests/pure2-bugfix-for-discard-precedence.cpp2 +++ b/regression-tests/pure2-bugfix-for-discard-precedence.cpp2 @@ -5,12 +5,12 @@ quantity: type = { _ = _; } operator+=: (inout this, that) -> forward quantity = { - _ = number += that.number; + number += that.number; return this; } } main: () = { x: quantity = (std::in_place, 1729); - _ = x += x; + x += x; } diff --git a/regression-tests/test-results/pure2-bugfix-for-discard-precedence.cpp b/regression-tests/test-results/pure2-bugfix-for-discard-precedence.cpp index 455a8a60fe..b695966734 100644 --- a/regression-tests/test-results/pure2-bugfix-for-discard-precedence.cpp +++ b/regression-tests/test-results/pure2-bugfix-for-discard-precedence.cpp @@ -19,7 +19,7 @@ class quantity { #line 7 "pure2-bugfix-for-discard-precedence.cpp2" - public: [[nodiscard]] auto operator+=(quantity const& that) -> quantity&; + public: auto operator+=(quantity const& that) -> quantity&; public: quantity(quantity const&) = delete; /* No 'that' constructor, suppress copy */ public: auto operator=(quantity const&) -> void = delete; @@ -42,14 +42,14 @@ auto main() -> int; static_cast(_); } - [[nodiscard]] auto quantity::operator+=(quantity const& that) -> quantity&{ - static_cast(number += that.number); + auto quantity::operator+=(quantity const& that) -> quantity&{ + number += that.number; return (*this); } #line 13 "pure2-bugfix-for-discard-precedence.cpp2" auto main() -> int{ quantity x {std::in_place, 1729}; - static_cast(x += std::move(x)); + x += std::move(x); } diff --git a/regression-tests/test-results/version b/regression-tests/test-results/version index bbb2e62cd2..196a577659 100644 --- a/regression-tests/test-results/version +++ b/regression-tests/test-results/version @@ -1,5 +1,5 @@ -cppfront compiler v0.2.1 Build 8810:1840 +cppfront compiler v0.2.1 Build 8811:0910 Copyright(c) Herb Sutter All rights reserved SPDX-License-Identifier: CC-BY-NC-ND-4.0 diff --git a/source/build.info b/source/build.info index 00aa0a41e0..cfca02584a 100644 --- a/source/build.info +++ b/source/build.info @@ -1 +1 @@ -"8810:1840" \ No newline at end of file +"8811:0910" \ No newline at end of file diff --git a/source/cppfront.cpp b/source/cppfront.cpp index 02c0658c42..f61a692a7e 100644 --- a/source/cppfront.cpp +++ b/source/cppfront.cpp @@ -5459,6 +5459,8 @@ class cppfront // -- for now there's no opt-out, wait and see whether we actually need one if ( func->has_non_void_return_type() + && !func->is_assignment() + && !func->is_compound_assignment() && ( printer.get_phase() == printer.phase1_type_defs_func_decls || n.has_initializer() // so we're printing it in phase 2 diff --git a/source/parse.h b/source/parse.h index 94432676ed..e96570807c 100644 --- a/source/parse.h +++ b/source/parse.h @@ -280,6 +280,12 @@ struct binary_expression_node // API // + auto terms_size() const + -> int + { + return std::ssize(terms); + } + auto is_identifier() const -> bool { @@ -1971,6 +1977,9 @@ struct function_type_node auto is_constructor_with_move_that() const -> bool; + auto is_compound_assignment() const + -> bool; + auto is_assignment() const -> bool; @@ -2844,6 +2853,16 @@ struct declaration_node return false; } + auto is_compound_assignment() const + -> bool + { + if (auto func = std::get_if(&type)) { + return (*func)->is_compound_assignment(); + } + // else + return false; + } + auto is_assignment() const -> bool { @@ -3298,6 +3317,33 @@ auto function_type_node::is_constructor_with_move_that() const } +auto function_type_node::is_compound_assignment() const + -> bool +{ + if ( + ( + my_decl->has_name("operator+=") + || my_decl->has_name("operator-=") + || my_decl->has_name("operator*=") + || my_decl->has_name("operator/=") + || my_decl->has_name("operator%=") + || my_decl->has_name("operator&=") + || my_decl->has_name("operator|=") + || my_decl->has_name("operator^=") + || my_decl->has_name("operator<<=") + || my_decl->has_name("operator>>=") + ) + && (*parameters).ssize() > 1 + && (*parameters)[0]->has_name("this") + && (*parameters)[0]->direction() == passing_style::inout + ) + { + return true; + } + return false; +} + + auto function_type_node::is_assignment() const -> bool { @@ -4555,7 +4601,7 @@ class parser { if (allow_angle_operators) { - return binary_expression ( + auto ret = binary_expression ( [this](token const& t, token const& next) -> token const* { if (is_assignment_operator(t.type())) { return &t; @@ -4573,13 +4619,27 @@ class parser }, [=,this]{ return logical_or_expression(allow_angle_operators); } ); + + if (ret && ret->terms_size() > 1) { + error("assignment cannot be chained - instead of 'c = b = a;', write 'b = a; c = b;'"); + return {}; + } + + return ret; } else { - return binary_expression ( + auto ret = binary_expression ( [](token const&, token const&) -> token const* { return nullptr; }, [=,this]{ return logical_or_expression(allow_angle_operators); } ); + + if (ret && ret->terms_size() > 1) { + error("assignment cannot be chained - instead of 'c = b = a;', write 'b = a; c = b;'"); + return {}; + } + + return ret; } }