Allow --Wdisable to take precedence over --Werror for warning messages

[kfcripps]

This reverts a change that was introduced by #4366, and is in fact redundant for the purpose of #4366 (it was originally necessary, but later became redundant after updates were made to that PR). Not only was it redundant, but it also introduced some undesired behavior (see #4365 (comment) and related discussion). Edit: See #4894 (comment)

For the following P4 program:

header h_t {
    bit<28> f1;
    bit<4> f2;
}

extern void __e(in bit<28> arg);

control C() {
    action foo() {
        h_t h;
        __e(h.f1);
    }

    table t {
        actions = { foo; }
        default_action = foo;
    }

    apply {
        t.apply();
    }
}

control proto();
package top(proto p);

top(C()) main;

The command-line options p4test tmp.p4 --dump tmp --Werror --Wdisable=uninitialized_use --Wdisable=invalid_header currently result in:

tmp.p4(28): [--Werror=invalid_header] error: accessing a field of an invalid header h
        __e(h.f1);
            ^
tmp.p4(28): [--Werror=uninitialized_use] error: h.f1 may be uninitialized
        __e(h.f1);
            ^^^^

but should result in no errors or warnings.

[ajwalton]

Should there be a test-case for this?

(From what I can see, the code is OK.)

[kfcripps]

Should there be a test-case for this?

(From what I can see, the code is OK.)

I do not know how to test command-line options using the existing infrastructure.

[ajwalton]

Should there be a test-case for this?
(From what I can see, the code is OK.)

I do not know how to test command-line options using the existing infrastructure.

Me neither, I'm afraid. @vlstill, @fruffy, @ChrisDodd ?

[kfcripps]

Apologies, I forgot about diagnostics that are both errors and warnings. The original changes I proposed in this PR will not work. @ajwalton Please see the new changes.

[fruffy]

Should there be a test-case for this?
(From what I can see, the code is OK.)

I do not know how to test command-line options using the existing infrastructure.

Me neither, I'm afraid. @vlstill, @fruffy, @ChrisDodd ?

You can set up tests in CMake with custom arguments like this: https://github.com/p4lang/p4c/blob/main/backends/p4test/CMakeLists.txt#L118

I do not recall whether this also supports reference files, however.

[kfcripps]

@ajwalton @fruffy Ok but it seems like the precedent before this PR was to not add such tests when changing the behavior of options such as e.g. --Werror / --Wdisable, so adding such tests would set a new precedent, which I believe is out of scope of this PR.

[kfcripps]

Sorry for all of the updates. The PR is now ready for review (and I will squash all commits when merging).

[asl]

LGTM, thanks!

[fruffy]

@ajwalton @fruffy Ok but it seems like the precedent before this PR was to not add such tests when changing the behavior of options such as e.g. --Werror / --Wdisable, so adding such tests would set a new precedent, which I believe is out of scope of this PR.

I think the precedent exists primarily because of neglect, not because of a conscious decision. We should consider building some test infrastructure that makes parameter/feature flag tests like these easier to add.

[kfcripps]

@ajwalton @fruffy Ok but it seems like the precedent before this PR was to not add such tests when changing the behavior of options such as e.g. --Werror / --Wdisable, so adding such tests would set a new precedent, which I believe is out of scope of this PR.

I think the precedent exists primarily because of neglect, not because of a conscious decision. We should consider building some test infrastructure that makes parameter/feature flag tests like these easier to add.

@fruffy I agree that we should build such testing mechanism. I am just saying that it can and should be done independently in a different PR.

For now I will be happy to manually generate and share the output of any combination of options.

[kfcripps]

@fruffy I agree that we should build such testing mechanism. I am just saying that it can and should be done independently in a different PR.

I opened #4907 for this purpose.

[fruffy]

@ajwalton @fruffy Ok but it seems like the precedent before this PR was to not add such tests when changing the behavior of options such as e.g. --Werror / --Wdisable, so adding such tests would set a new precedent, which I believe is out of scope of this PR.

I think the precedent exists primarily because of neglect, not because of a conscious decision. We should consider building some test infrastructure that makes parameter/feature flag tests like these easier to add.

@fruffy I agree that we should build such testing mechanism. I am just saying that it can and should be done independently in a different PR.

For now I will be happy to manually generate and share the output of any combination of options.

No problem, not requesting it for this particular PR but something we should definitely keep track of if it keeps coming up.

[kfcripps]

Does anyone else want to review this? If not, I'll probably merge this soon.