Skip to content

SEC-89 Fix messaget's copy-constructor and operator= #1464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion scripts/cpplint.py
Original file line number Diff line number Diff line change
Expand Up @@ -4876,7 +4876,9 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state,
CheckCommaSpacing(filename, clean_lines, linenum, error)
CheckBracesSpacing(filename, clean_lines, linenum, nesting_state, error)
CheckSpacingForFunctionCall(filename, clean_lines, linenum, error)
CheckCheck(filename, clean_lines, linenum, error)
# Disabled because whatever CHECK macro this was looking for, it isn't the
# CHECK macro used in Catch, but was complaining about it anyway.
#CheckCheck(filename, clean_lines, linenum, error)
CheckAltTokens(filename, clean_lines, linenum, error)
CheckAssert(filename, clean_lines, linenum, error)
classinfo = nesting_state.InnermostClass()
Expand Down
2 changes: 1 addition & 1 deletion src/solvers/refinement/string_refinement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ string_refinementt::string_refinementt(const infot &info):

/// display the current index set, for debugging
static void display_index_set(
messaget::mstreamt stream,
messaget::mstreamt &stream,
const namespacet &ns,
const index_set_pairt &index_set)
{
Expand Down
27 changes: 24 additions & 3 deletions src/util/message.h
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,17 @@ class messaget

messaget(const messaget &other):
message_handler(other.message_handler),
mstream(other.mstream)
mstream(other.mstream, *this)
{
}

messaget &operator=(const messaget &other)
{
message_handler=other.message_handler;
mstream.assign_from(other.mstream);
return *this;
}
Copy link
Contributor

@LAJW LAJW Oct 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you deleting copy-constructor and adding copy-assignment? Either none or both should be available. Follow The Rule of Five.

Copy link
Contributor Author

@smowton smowton Oct 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not. messaget's copy-constructor is fixed and gains a matching operator= (replacing its implicit one, which was also incorrect mirroring its existing explicit copy constructor); its internal class mstreamt is made neither copyable nor assignable (it should always move around with its enclosing messaget)


explicit messaget(message_handlert &_message_handler):
message_handler(&_message_handler),
mstream(M_DEBUG, *this)
Expand All @@ -177,13 +184,17 @@ class messaget
{
}

mstreamt(const mstreamt &other):
mstreamt(const mstreamt &other)=delete;

mstreamt(const mstreamt &other, messaget &_message):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is it useful for, it's not even tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Called from messaget's copy constructor.

message_level(other.message_level),
message(other.message),
message(_message),
source_location(other.source_location)
{
}

mstreamt &operator=(const mstreamt &other)=delete;

unsigned message_level;
messaget &message;
source_locationt source_location;
Expand Down Expand Up @@ -220,6 +231,16 @@ class messaget
{
return func(*this);
}

private:
void assign_from(const mstreamt &other)
{
message_level=other.message_level;
source_location=other.source_location;
// message, the pointer to my enclosing messaget, remains unaltered.
}

friend class messaget;
};

// Feeding 'eom' into the stream triggers
Expand Down
1 change: 1 addition & 0 deletions unit/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ SRC += unit_tests.cpp \
solvers/refinement/string_refinement/substitute_array_list.cpp \
util/expr_cast/expr_cast.cpp \
util/expr_iterator.cpp \
util/message.cpp \
util/simplify_expr.cpp \
catch_example.cpp \
# Empty last line
Expand Down
51 changes: 51 additions & 0 deletions unit/util/message.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*******************************************************************\

Module: Messaget tests

Author: Diffblue Limited. All rights reserved.

\*******************************************************************/

#include <testing-utils/catch.hpp>
#include <util/message.h>
#include <sstream>
#include <string.h>

TEST_CASE("Copy a messaget")
{
std::ostringstream sstream1, sstream2;
stream_message_handlert handler1(sstream1), handler2(sstream2);

messaget msg1(handler1);

// Copy messaget:
messaget msg2(msg1);

// Change its handler:
msg2.set_message_handler(handler2);

msg2.status() << "Test" << messaget::eom;

CHECK(sstream1.str()=="");
CHECK(sstream2.str()=="Test\n");
}

TEST_CASE("Assign a messaget")
{
std::ostringstream sstream1, sstream2;
stream_message_handlert handler1(sstream1), handler2(sstream2);

messaget msg1(handler1);

// Assign messaget:
messaget msg2;
msg2=msg1;

// Change its handler:
msg2.set_message_handler(handler2);

msg2.status() << "Test" << messaget::eom;

CHECK(sstream1.str()=="");
CHECK(sstream2.str()=="Test\n");
}