search

emacs-exwm / xelb

X protocol Emacs Lisp Binding
https://elpa.gnu.org/packages/xelb.html
GNU General Public License v3.0
27 stars 5 forks source link

Fix codegen with the latest xcb-proto and update #4

Closed Stebalien closed 9 months ago

Stebalien commented 9 months ago

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 commented 9 months ago

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

minad commented 9 months ago

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

Stebalien commented 9 months ago

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

Stebalien commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

SG. But I'd like to consider re-organizing this a bit before adding test files https://github.com/emacs-exwm/xelb/pull/11.

medranocalvo commented 9 months ago

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).

(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.)

  1. Improve type-name resolution.

This is great, thank you.

  1. 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.

  1. 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?

  1. Run make with xcb-proto 1.16.0.

Great, thank you.

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

Stebalien commented 9 months ago

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 commented 9 months ago

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

minad commented 9 months ago

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 commented 9 months ago

~~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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

medranocalvo commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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