Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve const correctness #3083

Merged
merged 1 commit into from
Feb 15, 2024
Merged

Improve const correctness #3083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 7 additions & 7 deletions unit_tests/test_falco_engine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class test_falco_engine : public ::testing::Test {

}

bool load_rules(std::string rules_content, std::string rules_filename)
bool load_rules(const std::string& rules_content, const std::string& rules_filename)
{
bool ret = false;
falco::load_result::rules_contents_t rc = {{rules_filename, rules_content}};
Expand All @@ -47,7 +47,7 @@ class test_falco_engine : public ::testing::Test {
}

// This must be kept in line with the (private) falco_engine::s_default_ruleset
uint64_t num_rules_for_ruleset(std::string ruleset = "falco-default-ruleset")
uint64_t num_rules_for_ruleset(const std::string& ruleset = "falco-default-ruleset")
{
return m_engine->num_rules_for_ruleset(ruleset);
}
Expand All @@ -57,14 +57,14 @@ class test_falco_engine : public ::testing::Test {
return m_load_result->has_warnings();
}

bool check_warning_message(std::string warning_msg)
bool check_warning_message(const std::string& warning_msg) const
{
if(!m_load_result->has_warnings())
{
return false;
}

for(auto &warn : m_load_result_json["warnings"])
for(const auto &warn : m_load_result_json["warnings"])
{
std::string msg = warn["message"];
// Debug:
Expand All @@ -78,15 +78,15 @@ class test_falco_engine : public ::testing::Test {
return false;
}

bool check_error_message(std::string error_msg)
bool check_error_message(const std::string& error_msg) const
{
// if the loading is successful there are no errors
if(m_load_result->successful())
{
return false;
}

for(auto &err : m_load_result_json["errors"])
for(const auto &err : m_load_result_json["errors"])
{
std::string msg = err["message"];
// Debug:
Expand All @@ -100,7 +100,7 @@ class test_falco_engine : public ::testing::Test {
return false;
}

std::string get_compiled_rule_condition(std::string rule_name = "")
std::string get_compiled_rule_condition(std::string rule_name = "") const
{
auto rule_description = m_engine->describe_rule(&rule_name, {});
return rule_description["rules"][0]["details"]["condition_compiled"].template get<std::string>();
Expand Down
12 changes: 6 additions & 6 deletions userspace/engine/evttype_index_ruleset.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ bool evttype_index_ruleset::ruleset_filters::run(sinsp_evt *evt, falco_rule& mat
{
if(evt->get_type() < m_filter_by_event_type.size())
{
for(auto &wrap : m_filter_by_event_type[evt->get_type()])
for(const auto &wrap : m_filter_by_event_type[evt->get_type()])
{
if(wrap->filter->run(evt))
{
Expand All @@ -126,7 +126,7 @@ bool evttype_index_ruleset::ruleset_filters::run(sinsp_evt *evt, falco_rule& mat
}

// Finally, try filters that are not specific to an event type.
for(auto &wrap : m_filter_all_event_types)
for(const auto &wrap : m_filter_all_event_types)
{
if(wrap->filter->run(evt))
{
Expand All @@ -144,7 +144,7 @@ bool evttype_index_ruleset::ruleset_filters::run(sinsp_evt *evt, std::vector<fal

if(evt->get_type() < m_filter_by_event_type.size())
{
for(auto &wrap : m_filter_by_event_type[evt->get_type()])
for(const auto &wrap : m_filter_by_event_type[evt->get_type()])
{
if(wrap->filter->run(evt))
{
Expand All @@ -160,7 +160,7 @@ bool evttype_index_ruleset::ruleset_filters::run(sinsp_evt *evt, std::vector<fal
}

// Finally, try filters that are not specific to an event type.
for(auto &wrap : m_filter_all_event_types)
for(const auto &wrap : m_filter_all_event_types)
{
if(wrap->filter->run(evt))
{
Expand All @@ -175,7 +175,7 @@ bool evttype_index_ruleset::ruleset_filters::run(sinsp_evt *evt, std::vector<fal
libsinsp::events::set<ppm_sc_code> evttype_index_ruleset::ruleset_filters::sc_codes()
{
libsinsp::events::set<ppm_sc_code> res;
for(auto &wrap : m_filters)
for(const auto &wrap : m_filters)
{
res.insert(wrap->sc_codes.begin(), wrap->sc_codes.end());
}
Expand All @@ -185,7 +185,7 @@ libsinsp::events::set<ppm_sc_code> evttype_index_ruleset::ruleset_filters::sc_co
libsinsp::events::set<ppm_event_code> evttype_index_ruleset::ruleset_filters::event_codes()
{
libsinsp::events::set<ppm_event_code> res;
for(auto &wrap : m_filters)
for(const auto &wrap : m_filters)
{
res.insert(wrap->event_codes.begin(), wrap->event_codes.end());
}
Expand Down
6 changes: 3 additions & 3 deletions userspace/engine/falco_common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ static std::vector<std::string> rule_matching_names = {
"all"
};

bool falco_common::parse_priority(std::string v, priority_type& out)
bool falco_common::parse_priority(const std::string& v, priority_type& out)
Copy link
Contributor

Choose a reason for hiding this comment

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

@federico-sysdig may we summon you again to help decide on a style more uniformly? For example have gotten feedback both ways, pass by value vs const ref ... and lately we transitioned to using std::string_view in libs. Thank you 🙏 .

Copy link
Contributor

Choose a reason for hiding this comment

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

TL;DR:
Pass by value all types that are cheap to copy (e.g. built-in types, but not only...) and pass by const reference the other ones.

There are other cases where you'll here advice to pass-by-value and then move or pass-by-rvalue-reference. I won't discuss them here. I think the matter gets more confused and difficult and a simple rule that is good enough is a superior choice. So, these cases apart an std::string should not be passed by value as it might not be "cheap" to copy, meaning it might require a heap allocation (unless it's very short, but small-string-optimization is another topic I'd leave out of the discussion).

Regarding std::string_view, the type is not built-in, clearly, but it's implemented as a pair of pointers (or a pointer and an integer, which is the same), thus it is cheap to copy and should be passed by value. See it as a more structured way to pass (char* str, int strlen), like we often do in C.

"Style" might come into play on the choice between const std::string& or std::string_view, but the latter came only recently with the library moving to C++17, so you won't see it very much. It is more efficient at times when the caller of the function uses a string literal as no temporary string gets created, so it is a better choice which I fully endorse, but, beware, it cannot be always used for passing the string buffer to a C function as it gives no guarantee of being null-terminated and it has some gotchas if you use it with an associative container whose key is an std::string (e.g. std::map<std::string, widget> C; and then, later on, C.find(mystringview)).

Briefly put, passing strings by const reference is fine, passing them through a std::string_view is even better, if you can afford it.

{
for (size_t i = 0; i < priority_names.size(); i++)
{
Expand All @@ -50,7 +50,7 @@ bool falco_common::parse_priority(std::string v, priority_type& out)
return false;
}

falco_common::priority_type falco_common::parse_priority(std::string v)
falco_common::priority_type falco_common::parse_priority(const std::string& v)
{
falco_common::priority_type out;
if (!parse_priority(v, out))
Expand Down Expand Up @@ -87,7 +87,7 @@ std::string falco_common::format_priority(priority_type v, bool shortfmt)
return out;
}

bool falco_common::parse_rule_matching(std::string v, rule_matching& out)
bool falco_common::parse_rule_matching(const std::string& v, rule_matching& out)
{
for (size_t i = 0; i < rule_matching_names.size(); i++)
{
Expand Down
6 changes: 3 additions & 3 deletions userspace/engine/falco_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ namespace falco_common
PRIORITY_DEBUG = 7
};

bool parse_priority(std::string v, priority_type& out);
priority_type parse_priority(std::string v);
bool parse_priority(const std::string& v, priority_type& out);
priority_type parse_priority(const std::string& v);
bool format_priority(priority_type v, std::string& out, bool shortfmt=false);
std::string format_priority(priority_type v, bool shortfmt=false);

Expand All @@ -68,5 +68,5 @@ namespace falco_common
ALL = 1
};

bool parse_rule_matching(std::string v, rule_matching& out);
bool parse_rule_matching(const std::string& v, rule_matching& out);
};
10 changes: 5 additions & 5 deletions userspace/engine/falco_engine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ static std::string fieldclass_key(const sinsp_filter_factory::filter_fieldclass_
return fld_info.name + fld_info.shortdesc;
}

void falco_engine::list_fields(std::string &source, bool verbose, bool names_only, bool markdown) const
void falco_engine::list_fields(const std::string &source, bool verbose, bool names_only, bool markdown) const
{
// Maps from field class name + short desc to list of event
// sources for which this field class can be used.
Expand Down Expand Up @@ -360,7 +360,7 @@ uint64_t falco_engine::num_rules_for_ruleset(const std::string &ruleset)
return ret;
}

void falco_engine::evttypes_for_ruleset(std::string &source, std::set<uint16_t> &evttypes, const std::string &ruleset)
void falco_engine::evttypes_for_ruleset(const std::string &source, std::set<uint16_t> &evttypes, const std::string &ruleset)
{
find_source(source)->ruleset->enabled_evttypes(evttypes, find_ruleset_id(ruleset));
}
Expand Down Expand Up @@ -422,7 +422,7 @@ std::unique_ptr<std::vector<falco_engine::rule_result>> falco_engine::process_ev
}

auto res = std::make_unique<std::vector<falco_engine::rule_result>>();
for(auto rule : source->m_rules)
for(const auto& rule : source->m_rules)
{
rule_result rule_result;
rule_result.evt = ev;
Expand Down Expand Up @@ -1025,7 +1025,7 @@ void falco_engine::fill_engine_state_funcs(filter_ruleset::engine_state_funcs &e
{
engine_state.get_ruleset = [this](const std::string &source_name, std::shared_ptr<filter_ruleset> &ruleset) -> bool
{
falco_source *src = m_sources.at(source_name);
const falco_source *src = m_sources.at(source_name);
if(src == nullptr)
{
return false;
Expand Down Expand Up @@ -1055,7 +1055,7 @@ void falco_engine::set_sampling_multiplier(double sampling_multiplier)
m_sampling_multiplier = sampling_multiplier;
}

void falco_engine::set_extra(std::string &extra, bool replace_container_info)
void falco_engine::set_extra(const std::string &extra, bool replace_container_info)
{
m_extra = extra;
m_replace_container_info = replace_container_info;
Expand Down
6 changes: 3 additions & 3 deletions userspace/engine/falco_engine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class falco_engine

// Print to stdout (using printf) a description of each field supported by this engine.
// If source is non-empty, only fields for the provided source are printed.
void list_fields(std::string &source, bool verbose, bool names_only, bool markdown) const;
void list_fields(const std::string &source, bool verbose, bool names_only, bool markdown) const;

// Provide an alternate rule reader, collector, and compiler
// to compile any rules provided via load_rules*
Expand Down Expand Up @@ -168,7 +168,7 @@ class falco_engine
// add k8s/container information to outputs when
// available.
//
void set_extra(std::string &extra, bool replace_container_info);
void set_extra(const std::string &extra, bool replace_container_info);

// Represents the result of matching an event against a set of
// rules.
Expand Down Expand Up @@ -271,7 +271,7 @@ class falco_engine
// typing-improved `enabled_event_codes` and `enabled_sc_codes` instead
// todo(jasondellaluce): remove this in future code refactors
//
void evttypes_for_ruleset(std::string &source,
void evttypes_for_ruleset(const std::string &source,
std::set<uint16_t> &evttypes,
const std::string &ruleset = s_default_ruleset);

Expand Down
2 changes: 1 addition & 1 deletion userspace/engine/filter_ruleset.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ limitations under the License.

#include "filter_ruleset.h"

void filter_ruleset::set_engine_state(filter_ruleset::engine_state_funcs& engine_state)
void filter_ruleset::set_engine_state(const filter_ruleset::engine_state_funcs& engine_state)
{
m_engine_state = engine_state;
}
Expand Down
2 changes: 1 addition & 1 deletion userspace/engine/filter_ruleset.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class filter_ruleset

virtual ~filter_ruleset() = default;

void set_engine_state(engine_state_funcs &engine_state);
void set_engine_state(const engine_state_funcs &engine_state);
engine_state_funcs &get_engine_state();

/*!
Expand Down
6 changes: 3 additions & 3 deletions userspace/engine/formats.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ falco_formats::~falco_formats()
}

std::string falco_formats::format_event(sinsp_evt *evt, const std::string &rule, const std::string &source,
const std::string &level, const std::string &format, std::set<std::string> &tags,
const std::string &level, const std::string &format, const std::set<std::string> &tags,
const std::string &hostname) const
{
std::string line;
Expand Down Expand Up @@ -101,7 +101,7 @@ std::string falco_formats::format_event(sinsp_evt *evt, const std::string &rule,
}
else
{
for (auto &tag : tags)
for (const auto &tag : tags)
{
rule_tags[rule_tags_idx++] = tag;
}
Expand All @@ -128,7 +128,7 @@ std::string falco_formats::format_event(sinsp_evt *evt, const std::string &rule,
line = full_line;
}

return line.c_str();
return line;
Copy link
Contributor

Choose a reason for hiding this comment

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

ohhh

}

std::map<std::string, std::string> falco_formats::get_field_values(sinsp_evt *evt, const std::string &source,
Expand Down
2 changes: 1 addition & 1 deletion userspace/engine/formats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class falco_formats
virtual ~falco_formats();

std::string format_event(sinsp_evt *evt, const std::string &rule, const std::string &source,
const std::string &level, const std::string &format, std::set<std::string> &tags,
const std::string &level, const std::string &format, const std::set<std::string> &tags,
const std::string &hostname) const;

std::map<std::string, std::string> get_field_values(sinsp_evt *evt, const std::string &source,
Expand Down
4 changes: 2 additions & 2 deletions userspace/engine/rule_loader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ std::string rule_loader::context::as_string()

bool first = true;

for(auto& loc : m_locs)
for(const auto& loc : m_locs)
{
os << (first ? "In " : " ");
first = false;
Expand Down Expand Up @@ -174,7 +174,7 @@ nlohmann::json rule_loader::context::as_json()
throw falco_exception("rule_loader::context without location?");
}

for(auto& loc : m_locs)
for(const auto& loc : m_locs)
{
nlohmann::json jloc, jpos;

Expand Down
12 changes: 6 additions & 6 deletions userspace/engine/rule_loader_collector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,15 @@ static void validate_exception_info(
THROW(ex.fields.items.size() != ex.comps.items.size(),
"Fields and comps lists must have equal length",
ex.ctx);
for (auto &v : ex.comps.items)
for (const auto &v : ex.comps.items)
{
THROW(!is_operator_defined(v.item),
std::string("'") + v.item + "' is not a supported comparison operator",
ex.ctx);
}
if (source)
{
for (auto &v : ex.fields.items)
for (const auto &v : ex.fields.items)
{
THROW(!source->is_field_defined(v.item),
std::string("'") + v.item + "' is not a supported filter field",
Expand Down Expand Up @@ -212,12 +212,12 @@ void rule_loader::collector::append(configuration& cfg, macro_info& info)

void rule_loader::collector::define(configuration& cfg, rule_info& info)
{
auto prev = m_rule_infos.at(info.name);
const auto* prev = m_rule_infos.at(info.name);
THROW(prev && prev->source != info.source,
"Rule has been re-defined with a different source",
info.ctx);

auto source = cfg.sources.at(info.source);
const auto* source = cfg.sources.at(info.source);
if (!source)
{
info.unknown_source = true;
Expand Down Expand Up @@ -248,7 +248,7 @@ void rule_loader::collector::append(configuration& cfg, rule_update_info& info)

// note: source can be nullptr in case we've collected a
// rule for which the source is unknown
falco_source* source = nullptr;
const falco_source* source = nullptr;
if (!prev->unknown_source)
{
// note: if the source is not unknown, this should not return nullptr
Expand Down Expand Up @@ -330,7 +330,7 @@ void rule_loader::collector::selective_replace(configuration& cfg, rule_update_i

// note: source can be nullptr in case we've collected a
// rule for which the source is unknown
falco_source* source = nullptr;
const falco_source* source = nullptr;
if (!prev->unknown_source)
{
// note: if the source is not unknown, this should not return nullptr
Expand Down
2 changes: 1 addition & 1 deletion userspace/engine/rule_loader_compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ void rule_loader::compiler::compile_macros_infos(
filter_macro_resolver macro_resolver;
for (auto &m : out)
{
auto info = macro_info_from_name(col, m.name);
const auto* info = macro_info_from_name(col, m.name);
resolve_macros(macro_resolver, col.macros(), out, m.condition, info->cond, info->visibility, info->ctx);
}
}
Expand Down
4 changes: 2 additions & 2 deletions userspace/engine/rule_loader_reader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ static void read_rule_exceptions(
rule_loader::context vals_ctx(exvals, rule_loader::context::EXCEPTION_VALUES, "", ex_ctx);
THROW(!exvals.IsSequence(),
"Rule exception values must be a sequence", vals_ctx);
for (auto &val : exvals)
for (const auto &val : exvals)
{
rule_loader::context vctx(val, rule_loader::context::EXCEPTION_VALUE, "", vals_ctx);
rule_loader::rule_exception_info::entry v_ex_val;
Expand Down Expand Up @@ -654,7 +654,7 @@ void rule_loader::reader::read_item(
}

// if any expected key has not been defined throw an error
for (auto &key : expected_keys) {
for (const auto &key : expected_keys) {
rule_loader::context keyctx(item[key], rule_loader::context::OVERRIDE, key, ctx);
THROW(true, "Unexpected key '" + key + "': no corresponding entry under 'override' is defined.", keyctx);
}
Expand Down