will add this to the agenda for today

@apinski-cavium, actually, I had inverted the definitions -- local compile-time known is more restrictive. See my latest commit which fixes this and adopts your streamlining suggestions.

@apinski-cavium, actually, I had inverted the definitions -- local compile-time known is more restrictive. See my latest commit which fixes this and adopts your streamlining suggestions.

Yes I didn't even noticed that until now and yes this is now +1 from me.

Would be nice for Petra to validate this.

Anyone else have comments or suggestions on this one?

Any further comments on this or should we merge it?

I do have some open questions about generics and literals.

Sure. Can you provide a pointer to those questions? I might be missing something but I don't see them in the conversation on this issue.

Looks like I forgot to submit the comments. Somehow github has preserved them for a long time...

@jnfoster, let me know when this is ready for me to do a review.

By a very quick test, it seems that the compiler does reject constructor parameters as well as generic-parameter-derived values in type declarations:

int<4> fn<T>(inout bit<4> data) {
    int<(T.minSizeInBits())> v = 42;
    v = v + data;
    return v;
}

control C(inout bit<4> data)(int cnt) {
    int<(cnt)> var;

    apply {
    }
}

# ./p4test test.p4
test.p4(2): [--Werror=expected] error: T.minSizeInBits(): expected a constant
    int<(T.minSizeInBits())> v = 42;
         ^^^^^^^^^^^^^^^^^
test.p4(8): [--Werror=expected] error: cnt: expected a constant
    int<(cnt)> var;
         ^^^

So at least this one case should be safe and we would not be restricting what the compiler already compiles.

However, minSizeInBits is currently not considered a constant also where it should be according to this spec:

typedef int<4> T;

int<4> fn(inout bit<4> data) {
    int<(T.minSizeInBits())> v = 42;
    v = v + data;
    return v;
}

const int cnt = 4;

control C(inout bit<4> data) {
    int<(cnt)> var;

    apply {
    }
}

$ ./p4test test.p4
test.p4(4): [--Werror=expected] error: T.minSizeInBits(): expected a constant
    int<(T.minSizeInBits())> v = 42;
         ^^^^^^^^^^^^^^^^^

Here is a small example showing what p4c does.

control C(inout bit<8> x);
package P(C c);

const int f = 42;

int g(int x) { return x; }

control MyB(inout bit<8> x, inout bit<8> y)(int n) {
  bit<(1 + 1)> a = 0;
  bit<(f)> b = 1;
  bit<(f / 2)> c = 2;
  // These are all currently errors, and they should be
  // bit<(g(3))> d = 3;
  // bit<(n)> e = 3;
  // bit<(x)> f = 4;
  apply {
    x = y;
  }
}

control MyC(inout bit<8> x)(int o) {
  MyB(o) b;
  bit<8> y = 0;
  apply {
    b.apply(x, y);
  }
}

P(MyC(9)) main;

@jonathan-dilorenzo this is ready for you to take a look.

Actually @jnfoster, can you say why you also wanted to remove the experimenting with bison file? Doesn't look obviously related to this...

Good catch. I've fixed that.

(Sorry for the force-pushes. I was trying to rebase and squash these to tidy it up, which is perhaps ill-advised on a public PR.)

Np! Looks good to me. Merging.

@jnfoster @ChrisDodd If Nate agrees that some of Chris Dodd's latest comments/questions justify changes, it would be good to create a separate issue to track those.

@jnfoster @ChrisDodd If Nate agrees that some of Chris Dodd's latest comments/questions justify changes, it would be good to create a separate issue to track those.

The suggestions are good, but creating an issue for something that can be implemented in a few minutes seems like overkill. I created #1286. If people agree, I think it would be safe without discussing this at the LDWG -- all of the changes only increase clarity, but don't change meaning.

Of course, we can also discuss it, so we feel good about the process and about PRs being applied :-)

Made an executive decision to merge. Obvious and simple improvements aren't worth the discussion imo, but feel free to push back if you feel otherwise.