Skip to content

Commit

Permalink
Clarify lifetime of defines maps
Browse files Browse the repository at this point in the history
Wrap the bare pointer in a unique_ptr and move it into the sink.
  • Loading branch information
AI0867 committed Jan 16, 2018
1 parent f453f14 commit 5df4068
Showing 1 changed file with 8 additions and 8 deletions.
16 changes: 8 additions & 8 deletions src/serialization/preprocessor.cpp
Expand Up @@ -699,7 +699,7 @@ class preprocessor_data : public preprocessor
int line,
const std::string& dir,
const std::string& domain,
std::map<std::string, std::string>* defines,
std::unique_ptr<std::map<std::string, std::string>> defines,
bool is_define = false);

virtual bool get_chunk() override;
Expand Down Expand Up @@ -797,14 +797,14 @@ preprocessor_data::preprocessor_data(preprocessor_streambuf& t,
int linenum,
const std::string& directory,
const std::string& domain,
std::map<std::string, std::string>* defines,
std::unique_ptr<std::map<std::string, std::string>> defines,

This comment has been minimized.

Copy link
@GregoryLundberg

GregoryLundberg Jan 17, 2018

Contributor

Codacy says:

src/serialization/preprocessor.cpp
Function parameter 'defines' should be passed by reference.
800 std::unique_ptr<std::map<std::string, std::string>> defines,

And, yes, it also says this commit made the leak warning go away.

This comment has been minimized.

Copy link
@AI0867

AI0867 Jan 17, 2018

Author Member

And this warning is just stupid. You should (almost) never pass a unique_ptr by reference. The type of this argument is exactly what I intended it to be, and properly indicates the semantics.

This comment has been minimized.

Copy link
@GregoryLundberg

GregoryLundberg Jan 17, 2018

Contributor

When it comes to a unique_ptr, I would agree. Somehow, I doubt cppcheck distinguishes that class from others; which would make this a true false positive. I note that the vast majority of occurrences for this comment are for std::string where I would argue it has a point and pass by reference would be a bit more performant with little to no change to readablity.

bool is_define)
: preprocessor(t)
, in_scope_(std::move(i))
, in_(*in_scope_)
, directory_(directory)
, strings_()
, local_defines_(defines)
, local_defines_(std::move(defines))
, tokens_()
, slowpath_(0)
, skipping_(0)
Expand Down Expand Up @@ -1441,7 +1441,7 @@ bool preprocessor_data::get_chunk()
size_t nb_arg = strings_.size() - token.stack_pos - 1;
size_t optional_arg_num = 0;

std::map<std::string, std::string>* defines = new std::map<std::string, std::string>;
std::unique_ptr<std::map<std::string, std::string>> defines{new std::map<std::string, std::string>};
const std::string& dir = filesystem::directory_name(val.location.substr(0, val.location.find(' ')));

for(size_t i = 0; i < nb_arg; ++i) {
Expand Down Expand Up @@ -1490,11 +1490,11 @@ bool preprocessor_data::get_chunk()

filesystem::scoped_istream buffer{new std::istringstream(argument.second)};

std::map<std::string, std::string>* temp_defines = new std::map<std::string, std::string>;
std::unique_ptr<std::map<std::string, std::string>> temp_defines{new std::map<std::string, std::string>};
temp_defines->insert(defines->begin(), defines->end());

buf->add_preprocessor<preprocessor_data>(
std::move(buffer), val.location, "", val.linenum, dir, val.textdomain, temp_defines, false);
std::move(buffer), val.location, "", val.linenum, dir, val.textdomain, std::move(temp_defines), false);

std::ostringstream res;
res << in.rdbuf();
Expand Down Expand Up @@ -1524,7 +1524,7 @@ bool preprocessor_data::get_chunk()
DBG_PREPROC << "substituting macro " << symbol << '\n';

parent_.add_preprocessor<preprocessor_data>(
std::move(buffer), val.location, "", val.linenum, dir, val.textdomain, defines, true);
std::move(buffer), val.location, "", val.linenum, dir, val.textdomain, std::move(defines), true);
} else {
DBG_PREPROC << "substituting (slow) macro " << symbol << '\n';

Expand All @@ -1538,7 +1538,7 @@ bool preprocessor_data::get_chunk()
{
std::istream in(buf.get());
buf->add_preprocessor<preprocessor_data>(
std::move(buffer), val.location, "", val.linenum, dir, val.textdomain, defines, true);
std::move(buffer), val.location, "", val.linenum, dir, val.textdomain, std::move(defines), true);

res << in.rdbuf();
}
Expand Down

0 comments on commit 5df4068

Please sign in to comment.