Get rid of custom implementation of Utils::PathName #4721

asl · 2024-06-13T05:34:43Z

Switch to std::filesystem::path
Simplify and cleanup code aroung
Get rid of (some) cstrings around paths

fruffy

Conditional approval from my side. It'd be great others also look across this since it impacts back end code.

Also a lot of this code will affect path manipulation. We do not necessarily test that.

fruffy · 2024-06-13T13:00:06Z

backends/p4tools/modules/testgen/targets/bmv2/test/small-step/p4_asserts_parser_test.cpp

        // Subpath for bf-p4c-compilers.
-        options.file = cstring(sourcePath);
-        options.file += curFile;
+        // FIXME: what is the logic behind here?


I assume this code was written to also work with compilers with a slightly different structure, e.g. the bf-p4c. But not sure what the approach here was there is likely an simpler solution.

Well, the thing is: the code inside the access condition is equivalent to the code above: we just emitting sourcePath / curFile. It looks like some leftover from previous changes / refactoring.

Good question. The code on lines 59-60 and 63-64 of the original code are identical.

Since there's nothing unique about this code block, perhaps we should delete the entire if statement?

Hopefully yes, but I just do not know the story behind this code, so maybe something was lost at some point of time.

The oldest commit I can find for this file is #3568. It was at a different point in the tree then and was later moved in a cleanup.

The duplicated code in the if (access(...)) block was present then, so it's nothing that we've accidentally corrupted after it was added to the p4c codebase.

Yeah feel free to be aggressive with refactorings here. This code is eventually meant to go away anyway since the parser is replaced with the actual p4-constaints parser.

grg · 2024-06-17T21:04:37Z

frontends/common/parser_options.cpp

-    Util::PathName newName(filename.getBasename() + baseSuffix + "." + filename.getExtension());
-    auto result = Util::PathName(folder).join(newName.toString());
-    return result.toString();
+static std::filesystem::path makeFileName(const std::filesystem::path &folder,


Folder? What are we, Windows programmers? 🤣

(No change needed -- you're not introducing a new name.)

Yeah, likely the code was copied from somewhere else :)

grg · 2024-06-17T21:06:52Z

frontends/common/parser_options.cpp

 }

 bool ParserOptions::isv1() const { return langVersion == ParserOptions::FrontendVersion::P4_14; }

 void ParserOptions::dumpPass(const char *manager, unsigned seq, const char *pass,
                             const IR::Node *node) const {
    if (strncmp(pass, "P4::", 4) == 0) pass += 4;
-    cstring name = cstring(manager) + "_" + Util::toString(seq) + "_" + pass;
+    std::string name = absl::StrCat(manager, "_", seq, "_", pass);


It's a shame we can't just use + instead of having to invoke StrCat. But I know we can't with the current types of the elements being concatenated.

Yes. Some stdlib implementations even return proxy object as a result operator+ to make things a bit more efficient. But it's not the case here. Anyway, the past code created lots of cstrings for no particular case...

grg · 2024-06-17T21:09:08Z

frontends/common/parser_options.cpp

-            if (filename == "-") filename = "tmp.p4"_cs;
-
-            cstring fileName = makeFileName(dumpFolder, filename, suffix);
+            std::string suffix = absl::StrFormat("-%04zu-%s", ++dump_uid, name);


Can absl::StrFormat be replaced by std::format when we move to C++20?

Yes, that's the intention. They are not quite compatible, but good thing is that StrFormat uses printf-style POSIX format strings (so, no new syntax as with boost::format), plus have compile-time checking of them.

grg · 2024-06-17T21:16:06Z

backends/dpdk/control-plane/bfruntime_ext.cpp

-        auto fileext = cstring(progName.find("."));
-        progName = cstring(progName.replace(fileext, cstring::empty));
-        progName = progName.trim("/\t\n\r");
+        auto progName = options.file.stem();


Nice! Much more compact using std::filesystem::path.

Lot's of similar code everywhere. In different combinations and variations.

grg · 2024-06-17T21:24:04Z

backends/graphs/graphs.h

@@ -124,7 +124,7 @@ class Graphs : public Inspector {
        boost::edge_attribute_t, GraphvizAttributes,
        boost::property<boost::edge_name_t, cstring, boost::property<boost::edge_index_t, int>>>;
    using graphProperties = boost::property<
-        boost::graph_name_t, cstring,
+        boost::graph_name_t, std::string,


What's the impact of this switch? Is there any noticeable memory impact?

These are transient attributes, so it does make sense to store them inside graph, not on the side. Simplifies logs here and here.

grg · 2024-06-17T21:35:08Z

backends/p4tools/modules/testgen/targets/bmv2/test/small-step/p4_asserts_parser_test.cpp

        // Subpath for bf-p4c-compilers.
-        options.file = cstring(sourcePath);
-        options.file += curFile;
+        // FIXME: what is the logic behind here?


Good question. The code on lines 59-60 and 63-64 of the original code are identical.

Since there's nothing unique about this code block, perhaps we should delete the entire if statement?

grg · 2024-06-18T01:17:41Z

I should also add that I'm okay with the changes. Will give Vladimir some time in case he has any feedback.

Feel free to ping me if the merge is urgent and I'll approve.

asl · 2024-06-18T21:09:35Z

Ok, I removed that FIXME and duplicated code. @vlstill Will you please check when you have a chance?

grg · 2024-06-19T19:39:03Z

Ok, I removed that FIXME and duplicated code. @vlstill Will you please check when you have a chance?

I'll approve tomorrow if we don't hear back from Vladimir. Feel free to ping me if I seem to have forgotten 🙂

grg

Approved 🙂

- Switch to std::filesystem::path - Simplify and cleanup code aroung - Get rid of (some) cstrings around paths

asl · 2024-06-20T16:44:30Z

@fruffy it looks CI is unhappy:

#9 46.30 Failed to open connection to "system" message bus: Failed to connect to socket /var/run/dbus/system_bus_socket: No such file or directory
#9 46.40 Created symlink /etc/systemd/user/sockets.target.wants/pk-debconf-helper.socket → /usr/lib/systemd/user/pk-debconf-helper.socket.
#9 46.41 Setting up packagekit-tools (1.1.13-2ubuntu1.1) ...
#9 46.41 Setting up software-properties-common (0.99.9.12) ...
#9 46.52 Processing triggers for libc-bin (2.31-0ubuntu9.2) ...
#9 46.53 Processing triggers for dbus (1.12.16-2ubuntu2.3) ...
#9 46.56 + sudo add-apt-repository -uy ppa:ubuntu-toolchain-r/test
#9 46.56 + command add-apt-repository -uy ppa:ubuntu-toolchain-r/test
#9 46.56 + add-apt-repository -uy ppa:ubuntu-toolchain-r/test
#9 183.4 Cannot add PPA: 'ppa:~ubuntu-toolchain-r/ubuntu/test'.
#9 183.4 ERROR: '~ubuntu-toolchain-r' user or team does not exist.

fruffy · 2024-06-20T16:47:13Z

Seems like we are running into actions/runner-images#9679

Edit: Turns out this might just be an outage https://status.canonical.com/#/incident/KNms6QK9ewuzz-7xUsPsNylV20jEt5kyKsd8A-3ptQG9fRkoWF_tPwQo3xO6l_CBViNLhdmwjQX92RChV_TTHQ==

asl · 2024-06-20T16:50:11Z

Seems like we are running into actions/runner-images#9679

Something like this to be added:

sudo add-apt-repository ppa:ubuntu-toolchain-r/test -y
sudo apt-get update -y

?

fruffy · 2024-06-20T16:51:41Z

Seems like we are running into actions/runner-images#9679

Something like this to be added:
sudo add-apt-repository ppa:ubuntu-toolchain-r/test -y
sudo apt-get update -y
?

We already have this here: https://github.com/p4lang/p4c/blob/main/tools/ci-build.sh#L123

asl · 2024-06-20T16:58:22Z

Seems like we are running into actions/runner-images#9679

Something like this to be added:
sudo add-apt-repository ppa:ubuntu-toolchain-r/test -y
sudo apt-get update -y
?
We already have this here: https://github.com/p4lang/p4c/blob/main/tools/ci-build.sh#L123

It is guarded by if [[ "${DISTRIB_RELEASE}" == "18.04" ]], no? It seems we'd need this unconditionally now?

fruffy · 2024-06-20T17:01:02Z

Seems like we are running into actions/runner-images#9679

Something like this to be added:
sudo add-apt-repository ppa:ubuntu-toolchain-r/test -y
sudo apt-get update -y
?
We already have this here: https://github.com/p4lang/p4c/blob/main/tools/ci-build.sh#L123
It is guarded by if [[ "${DISTRIB_RELEASE}" == "18.04" ]], no? It seems we'd need this unconditionally now?

No I think the CI build is failing at that particular line. We will probably see the same behavior in the Ubuntu 18 run.

asl · 2024-06-20T17:05:29Z

Edit: Turns out this might just be an outage https://status.canonical.com/#/incident/KNms6QK9ewuzz-7xUsPsNylV20jEt5kyKsd8A-3ptQG9fRkoWF_tPwQo3xO6l_CBViNLhdmwjQX92RChV_TTHQ==

This ended ~2 hours ago. The failure is fresh.

asl added core Topics concerning the core segments of the compiler (frontend, midend, parser) breaking-change This change may break assumptions of compiler back ends. labels Jun 13, 2024

asl requested review from vlstill and fruffy June 13, 2024 05:34

asl mentioned this pull request Jun 13, 2024

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

Merged

fruffy reviewed Jun 13, 2024

View reviewed changes

asl requested a review from grg June 17, 2024 16:56

grg reviewed Jun 17, 2024

View reviewed changes

asl force-pushed the remove-pathname branch from b7f415f to 97e4ca7 Compare June 18, 2024 21:09

grg approved these changes Jun 20, 2024

View reviewed changes

asl added this pull request to the merge queue Jun 20, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 20, 2024

asl added this pull request to the merge queue Jun 20, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 20, 2024

asl added 3 commits June 20, 2024 09:43

Get rid of custom implementation of Utils::PathName.

a6a08a1

- Switch to std::filesystem::path - Simplify and cleanup code aroung - Get rid of (some) cstrings around paths

Get rid of Util::PathName alias

09b5472

Resolve a fixme

f6fa457

asl force-pushed the remove-pathname branch from 97e4ca7 to f6fa457 Compare June 20, 2024 16:43

fruffy added the run-ubuntu18 Use this tag to trigger a Ubuntu-18 CI run. label Jun 20, 2024

asl enabled auto-merge June 20, 2024 17:05

asl added this pull request to the merge queue Jun 20, 2024

Merged via the queue into p4lang:main with commit a03f846 Jun 20, 2024
16 checks passed

asl deleted the remove-pathname branch June 20, 2024 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of custom implementation of Utils::PathName #4721

Get rid of custom implementation of Utils::PathName #4721

asl commented Jun 13, 2024

fruffy left a comment

fruffy Jun 13, 2024

asl Jun 13, 2024

grg Jun 17, 2024 •

edited

Loading

asl Jun 17, 2024

grg Jun 18, 2024

fruffy Jun 19, 2024

grg Jun 17, 2024

asl Jun 17, 2024

grg Jun 17, 2024

asl Jun 17, 2024

grg Jun 17, 2024

asl Jun 17, 2024

grg Jun 17, 2024

asl Jun 17, 2024

grg Jun 17, 2024

asl Jun 17, 2024

grg Jun 17, 2024 •

edited

Loading

grg commented Jun 18, 2024

asl commented Jun 18, 2024

grg commented Jun 19, 2024

grg left a comment

asl commented Jun 20, 2024

fruffy commented Jun 20, 2024 •

edited

Loading

asl commented Jun 20, 2024 •

edited

Loading

fruffy commented Jun 20, 2024

asl commented Jun 20, 2024

fruffy commented Jun 20, 2024

asl commented Jun 20, 2024

Get rid of custom implementation of Utils::PathName #4721

Get rid of custom implementation of Utils::PathName #4721

Conversation

asl commented Jun 13, 2024

fruffy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grg Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grg Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

grg commented Jun 18, 2024

asl commented Jun 18, 2024

grg commented Jun 19, 2024

grg left a comment

Choose a reason for hiding this comment

asl commented Jun 20, 2024

fruffy commented Jun 20, 2024 • edited Loading

asl commented Jun 20, 2024 • edited Loading

fruffy commented Jun 20, 2024

asl commented Jun 20, 2024

fruffy commented Jun 20, 2024

asl commented Jun 20, 2024

grg Jun 17, 2024 •

edited

Loading

grg Jun 17, 2024 •

edited

Loading

fruffy commented Jun 20, 2024 •

edited

Loading

asl commented Jun 20, 2024 •

edited

Loading