From 33c827bdc76cfc94a45eee80baaf9ab999f6075a Mon Sep 17 00:00:00 2001 From: Metin Cakircali Date: Fri, 14 Feb 2025 15:01:51 +0100 Subject: [PATCH 1/2] fix(Type): memory leak --- src/metkit/mars/Type.cc | 107 +++++++++++++++++++++------------------- src/metkit/mars/Type.h | 23 ++++++--- 2 files changed, 74 insertions(+), 56 deletions(-) diff --git a/src/metkit/mars/Type.cc b/src/metkit/mars/Type.cc index a7c9e1f..5725263 100644 --- a/src/metkit/mars/Type.cc +++ b/src/metkit/mars/Type.cc @@ -8,11 +8,17 @@ * does it submit to any jurisdiction. */ -#include +#include "metkit/mars/Type.h" #include "metkit/mars/MarsExpandContext.h" #include "metkit/mars/MarsRequest.h" -#include "metkit/mars/Type.h" + +#include +#include +#include +#include +#include +#include namespace metkit::mars { @@ -23,13 +29,13 @@ std::ostream& operator<<(std::ostream& s, const ContextRule& r) { return s; } -void Context::add(ContextRule* rule) { - rules_.push_back(rule); +void Context::add(std::unique_ptr rule) { + rules_.emplace_back(std::move(rule)); } bool Context::matches(MarsRequest req) const { - for (ContextRule* r : rules_) { + for (const auto& r : rules_) { if (!r->matches(req)) { return false; } @@ -45,17 +51,21 @@ std::ostream& operator<<(std::ostream& s, const Context& c) { void Context::print(std::ostream& out) const { std::string sep; out << "Context["; - for(const ContextRule* r: rules_) { + for (const auto& r : rules_) { out << sep << *r; sep = ","; } out << "]"; } +namespace { + class Include : public ContextRule { public: Include(const std::string& k, const std::set& vv) : ContextRule(k), vals_(vv) {} + ~Include() override = default; + bool matches(MarsRequest req) const override { if (!req.has(key_)) { return false; @@ -67,10 +77,10 @@ class Include : public ContextRule { } return false; } + private: // methods - void print(std::ostream& out) const override { - out << "Include[key=" << key_ << ",vals=[" << vals_ << "]]"; - } + void print(std::ostream& out) const override { out << "Include[key=" << key_ << ",vals=[" << vals_ << "]]"; } + private: std::set vals_; }; @@ -78,6 +88,9 @@ class Include : public ContextRule { class Exclude : public ContextRule { public: Exclude(const std::string& k, const std::set& vv) : ContextRule(k), vals_(vv) {} + + ~Exclude() override = default; + bool matches(MarsRequest req) const override { if (!req.has(key_)) { return false; @@ -89,10 +102,10 @@ class Exclude : public ContextRule { } return true; } + private: // methods - void print(std::ostream& out) const override { - out << "Exclude[key=" << key_ << ",vals=[" << vals_ << "]]"; - } + void print(std::ostream& out) const override { out << "Exclude[key=" << key_ << ",vals=[" << vals_ << "]]"; } + private: std::set vals_; }; @@ -100,28 +113,26 @@ class Exclude : public ContextRule { class Undef : public ContextRule { public: Undef(const std::string& k) : ContextRule(k) {} - bool matches(MarsRequest req) const override { - return !req.has(key_); - } + + ~Undef() override = default; + + bool matches(MarsRequest req) const override { return req.has(key_); } + private: // methods - void print(std::ostream& out) const override { - out << "Undef[key=" << key_ << "]"; - } + void print(std::ostream& out) const override { out << "Undef[key=" << key_ << "]"; } }; class Def : public ContextRule { public: Def(const std::string& k) : ContextRule(k) {} - bool matches(MarsRequest req) const override { - return req.has(key_); - } + + bool matches(MarsRequest req) const override { return req.has(key_); } + private: // methods - void print(std::ostream& out) const override { - out << "Def[key=" << key_ << "]"; - } + void print(std::ostream& out) const override { out << "Def[key=" << key_ << "]"; } }; -ContextRule* parseRule(std::string key, eckit::Value r) { +std::unique_ptr parseRule(std::string key, eckit::Value r) { std::set vals; @@ -129,40 +140,44 @@ ContextRule* parseRule(std::string key, eckit::Value r) { for (size_t k = 0; k < r.size(); k++) { vals.insert(r[k]); } - return new Include(key, vals); + return std::make_unique(key, vals); } ASSERT(r.contains("op")); std::string op = r["op"]; ASSERT(op.size() == 1); switch (op[0]) { - case 'u': return new Undef(key); - case 'd': return new Def(key); + case 'u': + return std::make_unique(key); + case 'd': + return std::make_unique(key); case '!': ASSERT(r.contains("vals")); eckit::Value vv = r["vals"]; for (size_t k = 0; k < vv.size(); k++) { vals.insert(vv[k]); } - return new Exclude(key, vals); + return std::make_unique(key, vals); } return nullptr; } -Context* parseContext(eckit::Value c) { +std::unique_ptr parseContext(const eckit::Value c) { - Context* context = new Context{}; + auto context = std::make_unique(); - eckit::Value keys = c.keys(); + auto keys = c.keys(); - for (size_t j = 0; j < keys.size(); j++) { - std::string key = keys[j]; - ContextRule* r = parseRule(key, c[key]); - context->add(r); + for (size_t j = 0; j < keys.size(); ++j) { + const auto& key = keys[j]; + context->add(parseRule(key, c[key])); } + return context; } +} // namespace + //---------------------------------------------------------------------------------------------------------------------- Type::Type(const std::string& name, const eckit::Value& settings) : @@ -203,13 +218,13 @@ Type::Type(const std::string& name, const eckit::Value& settings) : vals.push_back(vv); } - Context* context = nullptr; + std::unique_ptr context; if (d.contains("context")) { context = parseContext(d["context"]); } else { - context = new Context; + context = std::make_unique(); } - defaults_.emplace(context, vals); + defaults_.emplace(std::move(context), vals); } } } @@ -231,8 +246,7 @@ Type::Type(const std::string& name, const eckit::Value& settings) : for (size_t i = 0; i < unsets.size(); i++) { eckit::Value u = unsets[i]; ASSERT(u.contains("context")); - Context* c = parseContext(u["context"]); - unsets_.emplace(c); + unsets_.emplace(parseContext(u["context"])); } } } @@ -278,19 +292,16 @@ class InSet { bool operator()(const std::string& s) const { return set_.find(s) != set_.end(); } }; -bool Type::matches(const std::vector& match, - const std::vector& values) const { +bool Type::matches(const std::vector& match, const std::vector& values) const { InSet in_set(match); return std::find_if(values.begin(), values.end(), in_set) != values.end(); } - std::ostream& operator<<(std::ostream& s, const Type& x) { x.print(s); return s; } - std::string Type::tidy(const MarsExpandContext& ctx, const std::string& value) const { std::string result = value; expand(ctx, result); @@ -374,7 +385,6 @@ const std::vector& Type::flattenValues(const MarsRequest& request) return request.values(name_); } - void Type::clearDefaults() { defaults_.clear(); } @@ -387,7 +397,6 @@ const std::string& Type::name() const { return name_; } - const std::string& Type::category() const { return category_; } @@ -421,8 +430,7 @@ void Type::check(const MarsExpandContext& ctx, const std::vector& v if (values.size() != s.size()) { std::cerr << "Duplicate values in " << name_ << " " << values; std::set seen; - for (std::vector::const_iterator k = values.begin(); k != values.end(); - ++k) { + for (std::vector::const_iterator k = values.begin(); k != values.end(); ++k) { if (seen.find(*k) != seen.end()) { std::cerr << ' ' << *k; } @@ -435,6 +443,5 @@ void Type::check(const MarsExpandContext& ctx, const std::vector& v } } - //---------------------------------------------------------------------------------------------------------------------- } // namespace metkit::mars diff --git a/src/metkit/mars/Type.h b/src/metkit/mars/Type.h index 13de44c..225ac64 100644 --- a/src/metkit/mars/Type.h +++ b/src/metkit/mars/Type.h @@ -16,8 +16,11 @@ #ifndef metkit_Type_H #define metkit_Type_H +#include +#include #include #include +#include #include "eckit/memory/Counted.h" #include "eckit/value/Value.h" @@ -32,7 +35,15 @@ class MarsExpandContext; class ContextRule { public: - ContextRule(const std::string& k); + ContextRule(const ContextRule&) = default; + ContextRule& operator=(const ContextRule&) = default; + ContextRule(ContextRule&&) = delete; + ContextRule& operator=(ContextRule&&) = delete; + + explicit ContextRule(const std::string& k); + + virtual ~ContextRule() = default; + virtual bool matches(MarsRequest req) const = 0; friend std::ostream& operator<<(std::ostream& s, const ContextRule& x); @@ -48,7 +59,7 @@ class ContextRule { class Context { public: - void add(ContextRule* rule); + void add(std::unique_ptr rule); bool matches(MarsRequest req) const; friend std::ostream& operator<<(std::ostream& s, const Context& x); @@ -56,7 +67,7 @@ class Context { private: // methods void print(std::ostream& out) const; private: - std::vector rules_; + std::vector> rules_; }; //---------------------------------------------------------------------------------------------------------------------- @@ -106,10 +117,10 @@ class Type : public eckit::Counted { bool multiple_; bool duplicates_; - std::map> defaults_; + std::map, std::vector> defaults_; std::optional> inheritance_; - std::map sets_; - std::set unsets_; + std::map, std::string> sets_; + std::set> unsets_; protected: // methods virtual ~Type() override; From 33552bde1f080fbf30de2e370ed5465effae5587 Mon Sep 17 00:00:00 2001 From: Emanuele Danovaro Date: Fri, 14 Feb 2025 16:18:48 +0000 Subject: [PATCH 2/2] for loop --- src/metkit/mars/Type.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/metkit/mars/Type.cc b/src/metkit/mars/Type.cc index 5725263..a9a7670 100644 --- a/src/metkit/mars/Type.cc +++ b/src/metkit/mars/Type.cc @@ -337,18 +337,18 @@ void Type::expand(const MarsExpandContext& ctx, std::vector& values std::set seen; - for (std::vector::const_iterator j = values.begin(); j != values.end(); ++j) { - std::string value = *j; + for (const std::string& val : values) { + std::string value = val; if (!expand(ctx, value)) { std::ostringstream oss; - oss << *this << ": cannot expand '" << *j << "'" << ctx; + oss << *this << ": cannot expand '" << val << "'" << ctx; throw eckit::UserError(oss.str()); } if (!duplicates_) { if (seen.find(value) != seen.end()) { std::ostringstream oss; - oss << *this << ": duplicated value '" << *j << "'" << ctx; + oss << *this << ": duplicated value '" << val << "'" << ctx; throw eckit::UserError(oss.str()); } seen.insert(value); @@ -430,12 +430,12 @@ void Type::check(const MarsExpandContext& ctx, const std::vector& v if (values.size() != s.size()) { std::cerr << "Duplicate values in " << name_ << " " << values; std::set seen; - for (std::vector::const_iterator k = values.begin(); k != values.end(); ++k) { - if (seen.find(*k) != seen.end()) { - std::cerr << ' ' << *k; + for (const std::string& val : values) { + if (seen.find(val) != seen.end()) { + std::cerr << ' ' << val; } - seen.insert(*k); + seen.insert(val); } std::cerr << std::endl;