Fix codegen with the latest xcb-proto and update

[Stebalien]

I started down this path because I wanted to be able to fix the ash/lsh issue then run make. But I
ran into a bunch of issues running make. This patch set fixes those issues:

1. Implement float & double types (for the GLX extension). Not the prettiest code, but it works...
2. Improve type-name resolution.
3. Handle <length> elements (I think).
4. Switch back to the Emacs 28 pretty-printer format (when on Emacs 29+).
5. Run make with xcb-proto 1.16.0.

See the commit messages for more detail, and review commit by commit.

[Stebalien]

The float conversion code is going to take some work...

[minad]

Thank you for working on this. Regenerating the files was also on my todo list.

[Stebalien]

Still working on writing an actually working IEEE754 implementation in elisp, but I'm getting closer.

[Stebalien]

Test cases:

(defconst f32-test-cases '((#x00000001 . 1.4012984643e-45)
                                (#x007fffff . 1.1754942107e-38)
                                (#x00800000 . 1.17549435083e-38)
                                (#x7f7fffff . 3.4028234664e38)
                                (#x3f7fffff . 0.999999940395355225)
                                (#x3f800000 . 1.0)
                                (#x3f800001 . 1.00000011920928955)
                                (#xc0000000 . -2.0)
                                (#x00000000 . 0.0)
                                (#x80000000 . -0.0)
                                (#x7f800000 . 1e+INF)
                                (#xff800000 . -1e+INF)
                                (#x40490fdb . 3.14159274101257324)
                                (#x3eaaaaab . 0.333333343267440796)))
(let ((ctr 0))
  (pcase-dolist (`(,b . ,f) f32-test-cases)
    (let ((to (xcb:-f32-to-binary32 f))
          (from (xcb:-binary32-to-f32 b)))
      (unless (= b to) (print (format "%d: %s -> %x != %x" ctr f to b)))
      (unless (= f from) (print (format "%d: %x -> %s != %s" ctr b from f))))
    (cl-incf ctr)))

(defconst f64-test-cases `((#x3ff0000000000000 . 1.0)
                                (#x3ff0000000000001 . 1.0000000000000002)
                                (#x3ff0000000000002 . 1.0000000000000004)
                                (#x4000000000000000 . 2.0)
                                (#xc000000000000000 . -2.0)
                                (#x4008000000000000 . 3.0)
                                (#x4010000000000000 . 4.0)
                                (#x4014000000000000 . 5.0)
                                (#x4018000000000000 . 6.0)
                                (#x4037000000000000 . 23.0)
                                (#x3f88000000000000 . 0.01171875)
                                (#x0000000000000001 . 4.9406564584124654e-324)
                                (#x000fffffffffffff . 2.2250738585072009e-308)
                                (#x0010000000000000 . 2.2250738585072014e-308)
                                (#x7fefffffffffffff . 1.7976931348623157e308)
                                (#x0000000000000000 . +0.0)
                                (#x8000000000000000 . -0.0)
                                (#x7ff0000000000000 . +1e+INF)
                                (#xfff0000000000000 . -1e+INF)
                                (#x3fd5555555555555 . ,(/ 1.0 3.0))
                                (#x400921fb54442d18 . ,float-pi)))
(let ((ctr 0))
  (pcase-dolist (`(,b . ,f) f64-test-cases)
    (let ((to (xcb:-f64-to-binary64 f))
          (from (xcb:-binary64-to-f64 b)))
      (unless (= b to) (print (format "%d: %s -> %x != %x" ctr f to b)))
      (unless (= f from) (print (format "%d: %x -> %s != %s" ctr b from f))))
    (cl-incf ctr)))

Ish... there are some rounding issues. I'll research unit testing in Elisp later unless you happen to have a system you prefer.

[minad]

Ish... there are some rounding issues. I'll research unit testing in Elisp later unless you happen to have a system you prefer.

I prefer ERT. This is the system used by Emacs itself and also by Compat, see https://github.com/emacs-compat/compat/blob/main/compat-tests.el. See also the Github workflow and Makefile there. I am at least somewhat familiar with ERT.

[Stebalien]

SG. But I'd like to consider re-organizing this a bit before adding test files #11.

[medranocalvo]

I started down this path because I wanted to be able to fix the ash/lsh issue then run make. But I ran into a bunch of issues running make. This patch set fixes those issues:

This is great as well. Thank you for tackling this.

I mentioned somewhere that I worked on lsh and ash. That was a false memory: I only had a look at it. As per the documentation the only difference between lsh and ash is how negative fixnums are treated as unsigned by lsh when COUNT is negative. We have to audit every use and consider whether a negative fixnum is possible and, in that case, whether lsh behaviour is desired (doubtful).

• xelb-parse-op: no problem
• xelb-parse-bit: no problem
• xcb:keysyms:keycode->keysym: (lsh modifiers -13): COUNT is negative, can modifers be negative? And, in that case, is the lsh behaviour desired?
• xcb:keysyms:keycode->keysym: (lsh group-info -13): no problem, group-info cannot be negative.
• xcb-types.el: lsh is not used for signed integers in the pack/unpack functions.
• xcb-types.el: xcb:-popcount: has a negative count, but seems to be used with CARD fields. Must be audited.

(The above is not very relevant to this PR...)

1. Implement float & double types (for the GLX extension). Not the prettiest code, but it works...

Can't review, sorry. (If you think my review is crucial let me know and I'll block some time to do it.)

2. Improve type-name resolution.

This is great, thank you.

3. Handle <length> elements (I think).

I find it disconcerting that the ~size field is output twice, once with :initarg and once with :initform. Could both :initarg and :initform be placed in a single form?

I'm not sure whether this is completely correct. When is :initform evaluated? have all depended upon fields (len for DeviceClass) been initialized by then?

In my code-in-progress I override the xcb:marshall for the messages that define <length> so that the expression is evaluated as late as possible. I believe that something must be done in unmarshall (or elsewhere in xcb.el?) so that ~size bytes are consumed no matter the length of the object.... This is an improvement, though, so please commit nevertheless. I'll post a PR (draft) as soon as possible and we can discuss further over there.

4. Switch back to the Emacs 28 pretty-printer format (when on Emacs 29+).

This is good, thank you. Switching to the new format should happen in a separate commit. Not sure when we should do that. Now?

5. Run make with xcb-proto 1.16.0.

Great, thank you.

See the commit messages for more detail, and review commit by commit.

[Stebalien]

Implement float & double types (for the GLX extension). Not the prettiest code, but it works...

Can't review, sorry. (If you think my review is crucial let me know and I'll block some time to do it.)

Don't even try, that's why I wrote the tests.

I find it disconcerting that the ~size field is output twice, once with :initarg and once with :initform. Could both :initarg and :initform be placed in a single form?

Probably... I thought I did it that way mimicking other code, but I can't remember now.

I'm actually going to do what xcb:-enum does (kind of) and declare size in the base class with an initform of nil.

I'm not sure whether this is completely correct. When is :initform evaluated? have all depended upon fields (len for DeviceClass) been initialized by then?

My code is just wrong. The slot will be bound to the initform unevaluated, but:

1. It still needs to be evaluated (lazily).
2. xcb:unmarshal for xcb:-struct needs to handle it.

[Stebalien]

(new code should be fixed, but I won't be able to test until tonight)

[minad]

I mentioned somewhere that I worked on lsh and ash.

Regarding the ash, lsh changes, I also inspected the code briefly and I haven't found suspicious uses of lsh, but I will check again carefully.

This is good, thank you. Switching to the new format should happen in a separate commit. Not sure when we should do that. Now?

I wouldn't mind changing to a the format. The generator script could then check if emacs-major-version is 29.

I am a bit short on time right now, but I may be able to give this PR a thorough test later and also review it in greater detail.

[medranocalvo]

~size in xcb:unmarshall must be evaluated carefully: union uses it as well. Sorry, I see it's an struct method.

Possiblyxcb:marshall should eval it as well and pad the resulting message to be at least that size.

Possibly xcb:unmarshall should consume additional bytes if less than ~size.

[Stebalien]

Possiblyxcb:marshall should eval it as well and pad the resulting message to be at least that size.

I don't think that's necessary. We'd have to set an optional field (e.g., switch) we don't understand (which doesn't make sense). If we're writing the datastructure, we'll know the length.

From the spec:

This makes it possible to handle structures with conditional fields (see the element) where the future revisions of protocols may introduce new variants and old code must still properly ignore them.

---

Possibly xcb:unmarshall should consume additional bytes if less than ~size.

That should be what's happening here (by returning the size, same as for unions).

[Stebalien]

Ok, I've committed the float tests. We still need to wire everything up to makefiles and CI, but I'm going to do that in a separate PR after this one lands.

Everything should be good for a final round of reviews.

[minad]

@Stebalien Thanks, this looks all good to me. I added a few comments and questions.

[medranocalvo]

Possiblyxcb:marshall should eval it as well and pad the resulting message to be at least that size.

I don't think that's necessary. We'd have to set an optional field (e.g., switch) we don't understand (which doesn't make sense). If we're writing the datastructure, we'll know the length.

From the spec:

This makes it possible to handle structures with conditional fields (see the element) where the future revisions of protocols may introduce new variants and old code must still properly ignore them.

You are right, the one constructing the message should make sure the right size is calculated (in xcb:xinput:DeviceClass one should adjust the len field as needed). The only case I think this could fail would be if one of the cases in the switch could not be expressed by the <length> expression. I would find it surprising if spec writers would do that... The xcb:marshall method of xcb:-union does extend the vector to fill the length, but there it's obviously needed. I defer this to your judgement.

Possibly xcb:unmarshall should consume additional bytes if less than ~size.

That should be what's happening here (by returning the size, same as for unions).

Yes, thank you.

[Stebalien]

You are right, the one constructing the message should make sure the right size is calculated (in xcb:xinput:DeviceClass one should adjust the len field as needed). The only case I think this could fail would be if one of the cases in the switch could not be expressed by the expression. I would find it surprising if spec writers would do that... The xcb:marshall method of xcb:-union does extend the vector to fill the length, but there it's obviously needed. I defer this to your judgement.

I think the correct answer here is to assert that the size is correct, not to zero fill. Unions are "fixed size", so we need to pad to the correct size.

I've pushed a commit that does that (and adds some more paranoid checks to other sizes). If I'm wrong here, we should notice pretty quickly.

[medranocalvo]

You are right, the one constructing the message should make sure the right size is calculated (in xcb:xinput:DeviceClass one should adjust the len field as needed). The only case I think this could fail would be if one of the cases in the switch could not be expressed by the expression. I would find it surprising if spec writers would do that... The xcb:marshall method of xcb:-union does extend the vector to fill the length, but there it's obviously needed. I defer this to your judgement.

I think the correct answer here is to assert that the size is correct, not to zero fill. Unions are "fixed size", so we need to pad to the correct size.

I've pushed a commit that does that (and adds some more paranoid checks to other sizes). If I'm wrong here, we should notice pretty quickly.

That sounds good, thank you. I'm running this branch right now and no issue came up. I'd say it's ready to merge, if you think so.

[Stebalien]

I think so, I haven't had any issues either.