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

cstring-related cleanup, switch to std::string_view for some cstring API #4716

Merged
merged 1 commit into from
Jun 21, 2024
Merged

cstring-related cleanup, switch to std::string_view for some cstring API #4716

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions backends/dpdk/dpdkArch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1697,12 +1697,12 @@ const IR::Node *CopyMatchKeysToSingleStruct::postorder(IR::KeyElement *element)
isHeader = true;
keyName = keyName.replace('.', '_');
keyName =
keyName.replace("h_"_cs, control->name.toString() + "_" + table->name.toString() + "_");
keyName.replace("h_", control->name.toString() + "_" + table->name.toString() + "_");
} else if (metaCopyNeeded) {
if (keyName.startsWith("m.")) {
keyName = keyName.replace('.', '_');
keyName = keyName.replace(
"m_"_cs, control->name.toString() + "_" + table->name.toString() + "_");
"m_", control->name.toString() + "_" + table->name.toString() + "_");
} else {
keyName = control->name.toString() + "_" + table->name.toString() + "_" + keyName;
}
Expand Down Expand Up @@ -1859,7 +1859,7 @@ const IR::P4Table *SplitP4TableCommon::create_member_table(const IR::P4Table *tb
hidden->add(new IR::Annotation(IR::Annotation::hiddenAnnotation, {}));
auto nameAnnon = tbl->getAnnotation(IR::Annotation::nameAnnotation);
cstring nameA = nameAnnon->getSingleString();
cstring memName = nameA.replace(cstring(nameA.findlast('.')), "."_cs + memberTableName);
cstring memName = nameA.replace(nameA.findlast('.'), "." + memberTableName);
hidden->addAnnotation(IR::Annotation::nameAnnotation, new IR::StringLiteral(memName), false);

IR::IndexedVector<IR::ActionListElement> memberActionList;
Expand Down Expand Up @@ -1894,7 +1894,7 @@ const IR::P4Table *SplitP4TableCommon::create_group_table(const IR::P4Table *tbl
hidden->add(new IR::Annotation(IR::Annotation::hiddenAnnotation, {}));
auto nameAnnon = tbl->getAnnotation(IR::Annotation::nameAnnotation);
cstring nameA = nameAnnon->getSingleString();
cstring selName = nameA.replace(cstring(nameA.findlast('.')), "."_cs + selectorTableName);
cstring selName = nameA.replace(nameA.findlast('.'), "." + selectorTableName);
hidden->addAnnotation(IR::Annotation::nameAnnotation, new IR::StringLiteral(selName), false);
IR::IndexedVector<IR::Property> selector_properties;
selector_properties.push_back(new IR::Property("selector", new IR::Key(selector_keys), false));
Expand Down
7 changes: 4 additions & 3 deletions backends/dpdk/dpdkContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,10 @@ void DpdkContextGenerator::addKeyField(Util::JsonArray *keyJson, const cstring n
const cstring nameAnnotation, const IR::KeyElement *key,
int position) {
auto *keyField = new Util::JsonObject();
cstring fieldName = cstring(name.findlast('.'));
auto instanceName = name.replace(fieldName, cstring::empty);
fieldName = fieldName.trim(".\t\n\r");
const auto *fieldNamePos = name.findlast('.');
auto instanceName = name.replace(fieldNamePos, "");
// FIXME: trim string_view
cstring fieldName = cstring(fieldNamePos).trim(".\t\n\r");
std::string keyName(nameAnnotation);
// Replace header stack indices hdr[<index>] with hdr$<index>.
std::regex hdrStackRegex(R"(\[([0-9]+)\])");
Expand Down
3 changes: 1 addition & 2 deletions backends/dpdk/dpdkContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,7 @@ struct SelectionTable {
max_n_members_per_group = n_members_expr->to<IR::Constant>()->asInt();
}
// Fetch associated member table handle
cstring actionDataTableName = tbl->name.originalName;
actionDataTableName = actionDataTableName.replace("_sel"_cs, cstring::empty);
cstring actionDataTableName = tbl->name.originalName.replace("_sel", "");
auto actionTableAttr = ::get(tableAttrmap, actionDataTableName);
bound_to_action_data_table_handle = actionTableAttr.tableHandle;
}
Expand Down
2 changes: 1 addition & 1 deletion backends/ebpf/ebpfParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ void StateTranslationVisitor::compileExtractField(const IR::Expression *expr,
if (expr->is<IR::Member>() && expr->to<IR::Member>()->expr->is<IR::PathExpression>() &&
isPointerVariable(
expr->to<IR::Member>()->expr->to<IR::PathExpression>()->path->name.name)) {
exprStr = exprStr.replace("."_cs, "->"_cs);
exprStr = exprStr.replace(".", "->");
}
cstring tmp = absl::StrFormat("(unsigned long long) %s.%s", exprStr, fieldName);

Expand Down
2 changes: 1 addition & 1 deletion backends/ebpf/psa/ebpfPsaDeparser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ void XDPIngressDeparserPSA::emitPreDeparser(CodeBuilder *builder) {
cstring conditionSendToTC =
"if (%s->clone || %s->multicast_group != 0 ||"
" (!%s->drop && %s->egress_port == 0 && !%s->resubmit && %s->multicast_group == 0)) "_cs;
conditionSendToTC = conditionSendToTC.replace("%s"_cs, istd->name);
conditionSendToTC = conditionSendToTC.replace("%s", istd->name.name.string_view());
builder->append(conditionSendToTC);
builder->blockStart();
builder->emitIndent();
Expand Down
38 changes: 19 additions & 19 deletions backends/ebpf/psa/ebpfPsaGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,10 +231,10 @@ void PSAEbpfGenerator::emitHelperFunctions(CodeBuilder *builder) const {
"}"_cs;
if (options.emitTraceMessages) {
forEachFunc = forEachFunc.replace(
"%trace_msg_no_elements%"_cs,
" bpf_trace_message(\"do_for_each: No elements found in list\\n\");\n"_cs);
"%trace_msg_no_elements%",
" bpf_trace_message(\"do_for_each: No elements found in list\\n\");\n");
} else {
forEachFunc = forEachFunc.replace("%trace_msg_no_elements%"_cs, ""_cs);
forEachFunc = forEachFunc.replace("%trace_msg_no_elements%", "");
}
builder->appendLine(forEachFunc);
builder->newline();
Expand All @@ -250,11 +250,11 @@ void PSAEbpfGenerator::emitHelperFunctions(CodeBuilder *builder) const {
"}"_cs;
if (options.emitTraceMessages) {
cloneFunction = cloneFunction.replace(
cstring("%trace_msg_redirect%"),
" bpf_trace_message(\"do_clone: cloning pkt, egress_port=%d, cos=%d\\n\", "_cs
"entry->egress_port, entry->class_of_service);\n"_cs);
"%trace_msg_redirect%",
" bpf_trace_message(\"do_clone: cloning pkt, egress_port=%d, cos=%d\\n\", "
"entry->egress_port, entry->class_of_service);\n");
} else {
cloneFunction = cloneFunction.replace("%trace_msg_redirect%"_cs, ""_cs);
cloneFunction = cloneFunction.replace("%trace_msg_redirect%", "");
}
builder->appendLine(cloneFunction);
builder->newline();
Expand Down Expand Up @@ -284,24 +284,24 @@ void PSAEbpfGenerator::emitHelperFunctions(CodeBuilder *builder) const {
" }"_cs;
if (options.emitTraceMessages) {
pktClonesFunc = pktClonesFunc.replace(
"%trace_msg_clone_requested%"_cs,
"%trace_msg_clone_requested%",
" bpf_trace_message(\"Clone#%d: pkt clone requested, session=%d\\n\", "
"caller_id, session_id);\n"_cs);
"caller_id, session_id);\n");
pktClonesFunc = pktClonesFunc.replace(
"%trace_msg_clone_failed%"_cs,
" bpf_trace_message(\"Clone#%d: failed to clone packet\", caller_id);\n"_cs);
"%trace_msg_clone_failed%",
" bpf_trace_message(\"Clone#%d: failed to clone packet\", caller_id);\n");
pktClonesFunc =
pktClonesFunc.replace("%trace_msg_no_session%"_cs,
pktClonesFunc.replace("%trace_msg_no_session%",
" bpf_trace_message(\"Clone#%d: session_id not found, "
"no clones created\\n\", caller_id);\n"_cs);
"no clones created\\n\", caller_id);\n");
pktClonesFunc = pktClonesFunc.replace(
"%trace_msg_cloning_done%"_cs,
" bpf_trace_message(\"Clone#%d: packet cloning finished\\n\", caller_id);\n"_cs);
"%trace_msg_cloning_done%",
" bpf_trace_message(\"Clone#%d: packet cloning finished\\n\", caller_id);\n");
} else {
pktClonesFunc = pktClonesFunc.replace("%trace_msg_clone_requested%"_cs, cstring::empty);
pktClonesFunc = pktClonesFunc.replace("%trace_msg_clone_failed%"_cs, cstring::empty);
pktClonesFunc = pktClonesFunc.replace("%trace_msg_no_session%"_cs, cstring::empty);
pktClonesFunc = pktClonesFunc.replace("%trace_msg_cloning_done%"_cs, cstring::empty);
pktClonesFunc = pktClonesFunc.replace("%trace_msg_clone_requested%", "");
pktClonesFunc = pktClonesFunc.replace("%trace_msg_clone_failed%", "");
pktClonesFunc = pktClonesFunc.replace("%trace_msg_no_session%", "");
pktClonesFunc = pktClonesFunc.replace("%trace_msg_cloning_done%", "");
}

builder->appendLine(pktClonesFunc);
Expand Down
25 changes: 12 additions & 13 deletions backends/ebpf/psa/ebpfPsaTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -910,22 +910,21 @@ cstring EBPFTablePSA::addPrefixFunc(bool trace) {

if (trace) {
addPrefixFunc = addPrefixFunc.replace(
"%trace_msg_prefix_map_fail%"_cs,
" bpf_trace_message(\"Prefixes map update failed\\n\");\n"_cs);
"%trace_msg_prefix_map_fail%",
" bpf_trace_message(\"Prefixes map update failed\\n\");\n");
addPrefixFunc = addPrefixFunc.replace(
"%trace_msg_tuple_update_fail%"_cs,
" bpf_trace_message(\"Tuple map update failed\\n\");\n"_cs);
"%trace_msg_tuple_update_fail%",
" bpf_trace_message(\"Tuple map update failed\\n\");\n");
addPrefixFunc = addPrefixFunc.replace(
"%trace_msg_tuple_update_success%"_cs,
" bpf_trace_message(\"Tuple map update succeed\\n\");\n"_cs);
addPrefixFunc =
addPrefixFunc.replace("%trace_msg_tuple_not_found%"_cs,
" bpf_trace_message(\"Tuple not found\\n\");\n"_cs);
"%trace_msg_tuple_update_success%",
" bpf_trace_message(\"Tuple map update succeed\\n\");\n");
addPrefixFunc = addPrefixFunc.replace(
"%trace_msg_tuple_not_found%", " bpf_trace_message(\"Tuple not found\\n\");\n");
} else {
addPrefixFunc = addPrefixFunc.replace("%trace_msg_prefix_map_fail%"_cs, ""_cs);
addPrefixFunc = addPrefixFunc.replace("%trace_msg_tuple_update_fail%"_cs, ""_cs);
addPrefixFunc = addPrefixFunc.replace("%trace_msg_tuple_update_success%"_cs, ""_cs);
addPrefixFunc = addPrefixFunc.replace("%trace_msg_tuple_not_found%"_cs, ""_cs);
addPrefixFunc = addPrefixFunc.replace("%trace_msg_prefix_map_fail%", "");
addPrefixFunc = addPrefixFunc.replace("%trace_msg_tuple_update_fail%", "");
addPrefixFunc = addPrefixFunc.replace("%trace_msg_tuple_update_success%", "");
addPrefixFunc = addPrefixFunc.replace("%trace_msg_tuple_not_found%", "");
}

return addPrefixFunc;
Expand Down
42 changes: 20 additions & 22 deletions backends/ebpf/psa/externs/ebpfPsaMeter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,38 +412,36 @@ cstring EBPFMeterPSA::meterExecuteFunc(bool trace, P4::ReferenceMap *refMap) {
"}\n"_cs;

if (trace) {
meterExecuteFunc = meterExecuteFunc.replace(cstring("%trace_msg_meter_green%"),
meterExecuteFunc = meterExecuteFunc.replace("%trace_msg_meter_green%",
" bpf_trace_message(\""
"Meter: GREEN\\n\");\n"_cs);
meterExecuteFunc = meterExecuteFunc.replace(cstring("%trace_msg_meter_yellow%"),
"Meter: GREEN\\n\");\n");
meterExecuteFunc = meterExecuteFunc.replace("%trace_msg_meter_yellow%",
" bpf_trace_message(\""
"Meter: YELLOW\\n\");\n"_cs);
meterExecuteFunc = meterExecuteFunc.replace(cstring("%trace_msg_meter_red%"),
"Meter: YELLOW\\n\");\n");
meterExecuteFunc = meterExecuteFunc.replace("%trace_msg_meter_red%",
" bpf_trace_message(\""
"Meter: RED\\n\");\n"_cs);
"Meter: RED\\n\");\n");
meterExecuteFunc =
meterExecuteFunc.replace(cstring("%trace_msg_meter_no_value%"),
meterExecuteFunc.replace("%trace_msg_meter_no_value%",
" bpf_trace_message(\"Meter: No meter value! "
"Returning default GREEN\\n\");\n"_cs);
"Returning default GREEN\\n\");\n");
meterExecuteFunc =
meterExecuteFunc.replace(cstring("%trace_msg_meter_execute_bytes%"),
" bpf_trace_message(\"Meter: execute BYTES\\n\");\n"_cs);
meterExecuteFunc.replace("%trace_msg_meter_execute_bytes%",
" bpf_trace_message(\"Meter: execute BYTES\\n\");\n");
meterExecuteFunc =
meterExecuteFunc.replace(cstring("%trace_msg_meter_execute_packets%"),
" bpf_trace_message(\"Meter: execute PACKETS\\n\");\n"_cs);
meterExecuteFunc.replace("%trace_msg_meter_execute_packets%",
" bpf_trace_message(\"Meter: execute PACKETS\\n\");\n");
} else {
meterExecuteFunc = meterExecuteFunc.replace(cstring("%trace_msg_meter_green%"), ""_cs);
meterExecuteFunc = meterExecuteFunc.replace(cstring("%trace_msg_meter_yellow%"), ""_cs);
meterExecuteFunc = meterExecuteFunc.replace(cstring("%trace_msg_meter_red%"), ""_cs);
meterExecuteFunc = meterExecuteFunc.replace(cstring("%trace_msg_meter_no_value%"), ""_cs);
meterExecuteFunc =
meterExecuteFunc.replace(cstring("%trace_msg_meter_execute_bytes%"), ""_cs);
meterExecuteFunc =
meterExecuteFunc.replace(cstring("%trace_msg_meter_execute_packets%"), ""_cs);
meterExecuteFunc = meterExecuteFunc.replace("%trace_msg_meter_green%", "");
meterExecuteFunc = meterExecuteFunc.replace("%trace_msg_meter_yellow%", "");
meterExecuteFunc = meterExecuteFunc.replace("%trace_msg_meter_red%", "");
meterExecuteFunc = meterExecuteFunc.replace("%trace_msg_meter_no_value%", "");
meterExecuteFunc = meterExecuteFunc.replace("%trace_msg_meter_execute_bytes%", "");
meterExecuteFunc = meterExecuteFunc.replace("%trace_msg_meter_execute_packets%", "");
}

meterExecuteFunc = meterExecuteFunc.replace(cstring("%meter_struct%"),
cstring("struct ") + getBaseStructName(refMap));
meterExecuteFunc =
meterExecuteFunc.replace("%meter_struct%", "struct " + getBaseStructName(refMap));

return meterExecuteFunc;
}
Expand Down
2 changes: 1 addition & 1 deletion backends/tc/ebpfCodeGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,7 @@ void PnaStateTranslationVisitor::compileExtractField(const IR::Expression *expr,
if (auto member = expr->to<IR::Member>()) {
if (auto pathExpr = member->expr->to<IR::PathExpression>()) {
if (isPointerVariable(pathExpr->path->name.name)) {
exprStr = exprStr.replace("."_cs, "->"_cs);
exprStr = exprStr.replace(".", "->");
}
}
}
Expand Down
42 changes: 21 additions & 21 deletions lib/cstring.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ limitations under the License.

#include "cstring.h"

#include "absl/strings/str_cat.h"
#include "absl/strings/str_replace.h"

#if HAVE_LIBGC
#include <gc_cpp.h>
#define IF_HAVE_LIBGC(X) X
Expand Down Expand Up @@ -214,32 +217,32 @@ cstring cstring::substr(size_t start, size_t length) const {
}

cstring cstring::replace(char c, char with) const {
if (isNullOrEmpty()) return *this;
char *dup = strdup(c_str());
for (char *p = dup; *p; ++p)
if (*p == c) *p = with;
return cstring(dup);
}

cstring cstring::replace(cstring search, cstring replace) const {
if (search.isNullOrEmpty() || isNullOrEmpty()) return *this;
cstring cstring::replace(std::string_view search, std::string_view replace) const {
if (search.empty() || isNullOrEmpty()) return *this;

return cstring(absl::StrReplaceAll(str, {{search, replace}}));
}

std::string s_str = str;
std::string s_search = search.str;
std::string s_replace = replace.str;
cstring cstring::trim(const char *ws) const {
if (isNullOrEmpty()) return *this;

size_t pos = 0;
while ((pos = s_str.find(s_search, pos)) != std::string::npos) {
s_str.replace(pos, s_search.length(), s_replace);
pos += s_replace.length();
}
return cstring(s_str);
const char *start = str + strspn(str, ws);
size_t len = strlen(start);
while (len > 0 && strchr(ws, start[len - 1])) --len;
return cstring(start, len);
}

cstring cstring::indent(size_t amount) const {
std::string spaces = "";
for (size_t i = 0; i < amount; i++) spaces += " ";
cstring spc = newline + spaces;
return cstring(spaces) + replace(newline, spc);
std::string spaces(amount, ' ');
std::string spc = "\n" + spaces;
return cstring(absl::StrCat(spaces, absl::StrReplaceAll(string_view(), {{"\n", spc}})));
}

// See https://stackoverflow.com/a/33799784/4538702
Expand Down Expand Up @@ -285,20 +288,17 @@ cstring cstring::escapeJson() const {
cstring cstring::toUpper() const {
std::string st = str;
std::transform(st.begin(), st.end(), st.begin(), ::toupper);
cstring ret = cstring::to_cstring(st);
return ret;
return cstring(st);
}

cstring cstring::toLower() const {
std::string st = str;
std::transform(st.begin(), st.end(), st.begin(), ::tolower);
cstring ret = cstring::to_cstring(st);
return ret;
return cstring(st);
}

cstring cstring::capitalize() const {
std::string st = str;
st[0] = ::toupper(st[0]);
cstring ret = cstring::to_cstring(st);
return ret;
return cstring(st);
}
10 changes: 2 additions & 8 deletions lib/cstring.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,17 +254,11 @@ class cstring {
}
cstring substr(size_t start, size_t length) const;
cstring replace(char find, char replace) const;
cstring replace(cstring find, cstring replace) const;
cstring replace(std::string_view find, std::string_view replace) const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be the only breaking change in this PR, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Though if before explicit-cstring-construction it was like replace("foo", "bar"), then after this PR it will start working again w/o changes :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I wonder why not return a std::string here. In many cases, we would not need the result to be cstring and if the caller needs it, they can always do that explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked this case, and there is a mixture of usages. So I decided to re-evaluate this when we'll disable implicit conversion to / from std::string – then I will catch all such places :)

cstring exceptLast(size_t count) { return substr(0, size() - count); }

// trim leading and trailing whitespace (or other)
cstring trim(const char *ws = " \t\r\n") const {
if (!str) return *this;
const char *start = str + strspn(str, ws);
size_t len = strlen(start);
while (len > 0 && strchr(ws, start[len - 1])) --len;
return cstring(start, len);
}
cstring trim(const char *ws = " \t\r\n") const;

// Useful singletons.
static cstring newline;
Expand Down
8 changes: 4 additions & 4 deletions test/gtest/cstring.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,10 @@ TEST(cstring, substr) {

TEST(cstring, replace) {
cstring c = "Original"_cs;
EXPECT_EQ(c.replace("in"_cs, "out"_cs), "Origoutal");
EXPECT_EQ(c.replace(""_cs, "out"_cs), c);
EXPECT_EQ(c.replace("i"_cs, "o"_cs), "Orogonal");
EXPECT_EQ(c.replace("i"_cs, ""_cs), "Orgnal");
EXPECT_EQ(c.replace("in", "out"), "Origoutal");
EXPECT_EQ(c.replace("", "out"), c);
EXPECT_EQ(c.replace("i", "o"), "Orogonal");
EXPECT_EQ(c.replace("i", ""), "Orgnal");
}

TEST(cstring, literalSuffix) {
Expand Down
4 changes: 2 additions & 2 deletions tools/ir-generator/irclass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ void IrDefinitions::generate(std::ostream &t, std::ostream &out, std::ostream &i
unsigned nkId = 3;
auto *irNamespace = IrNamespace::get(nullptr, "IR"_cs);
for (auto *cls : *getClasses())
t << " " << cls->qualified_name(irNamespace).replace("::"_cs, "_"_cs) << " = " << nkId++
t << " " << cls->qualified_name(irNamespace).replace("::", "_") << " = " << nkId++
<< ",\n";

// Add some specials:
Expand Down Expand Up @@ -443,7 +443,7 @@ void IrClass::generate_hdr(std::ostream &out) const {
auto *irNamespace = IrNamespace::get(nullptr, "IR"_cs);
if (kind != NodeKind::Nested) {
out << indent << "DECLARE_TYPEINFO_WITH_TYPEID(" << name
<< ", NodeKind::" << qualified_name(irNamespace).replace("::"_cs, "_"_cs);
<< ", NodeKind::" << qualified_name(irNamespace).replace("::", "_");
if (!concreteParent) out << ", " << (kind != NodeKind::Interface ? "Node" : "INode");
for (const auto *p : parentClasses) out << ", " << p->qualified_name(containedIn);
out << ");" << std::endl;
Expand Down
Loading