Improve diagnostics for StructExpression

[kfcripps]

No description provided.

[jafingerhut]

I have not reviewed the C++ changes in any detail, but I have looked at the changes in the error messages, and they look reasonable to me.

[fruffy]

I am not sure why a cstring could be null here. Let me debug this.

[kfcripps]

I am not sure why a cstring could be null here. Let me debug this.

Thanks!

[fruffy]

I found the reason for the crash. No problem with your code but there is a problem with the way cstrings are initialized.

If you just declare them cstring var; the internal character will be a nullptr and the cstring will be invalid. I think this is something that should be fix, not sure why the class is written this way.

Either way, applying this patch should fix the problem:

diff --git a/backends/p4tools/p4tools.def b/backends/p4tools/p4tools.def
index ee2066443..aec5bb13d 100644
--- a/backends/p4tools/p4tools.def
+++ b/backends/p4tools/p4tools.def
@@ -211,7 +211,7 @@ public:
     inline cstring concolicMethodName = "";
 
     toString {
-        cstring argumentStr;
+        cstring argumentStr = "";
         cstring sep = "";
         for (const auto *arg : *arguments) {
             argumentStr += sep + arg->toString();

[fruffy]

This should either be marked override or implemented in the form of

toString {
    cstring str = "{";
    if (!components.empty()) {
        cstring exprStr = components.at(0)->expression->toString();
        str += " " + components.at(0)->toString() + " = " + exprStr;
    }
    for (unsigned i = 1; i < size(); i++) {
        cstring exprStr = components.at(i)->expression->toString();
        str += ", " + components.at(i)->toString() + " = " + exprStr;
    }
    return str + " }";
}

like with other nodes.

[jafingerhut]

I have not reviewed the C++ changes in any detail, but I have looked at the changes in the error messages, and they look reasonable to me.

[fruffy]

@kfcripps Are you able to enable automerge for these PRs? If not, I can toggle it.

[kfcripps]

@fruffy How do I do that?

[fruffy]

The CI has this option, but I believe you need merge rights for this.

[kfcripps]

Yeah, I've never contributed here before, so I don't have any rights in this repo.

[fruffy]

I can enable it for the PRs, if you want me to.

[kfcripps]

Would that enable auto-merging for all future PRs, or do you have to enable it individually for each?

[vlstill]

It is for each PR separately. In general when you have no write access, you can ask someone to merge the PR for you once you are happy with it and it is approved. In its current state it will probably have to be updated/rebased before that can be done (you will probably see a button for that).

[kfcripps]

Ok, I don't see why not - please enable it for my PRs. Thanks

[kfcripps]

Rebased

[kfcripps]

Looks like the build_indirect job is stuck?