use enums

[andreas-abel]

This has no obvious advantage over the existing code.

[adsharma]

A straightforward benefit is type safety

https://paste.ubuntu.com/p/z8qFWXN7wR/

Of course more specific types need to be defined for every class of registers.

On Sun, Aug 1, 2021 at 8:44 AM Andreas Abel @.***> wrote:

Closed #2 https://github.com/andreas-abel/uiCA/pull/2.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/andreas-abel/uiCA/pull/2#event-5095594151, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFA2A6YJSJH6M5OSVPQAZDT2VTXPANCNFSM5BJTMY5A .

[andreas-abel]

And obvious downsides in this case are that it reduces readability, frequently calling "all(cls)" likely reduces performance, and it makes the code completely inconsistent if only general-purpose registers are represented as enums but other register types are not.

Your modification

-    def __init__(self, reg, isImplicitStackOperand=False):
+    def __init__(self, reg: GPRegs, isImplicitStackOperand=False):
         self.reg = reg
         self.isImplicitStackOperand = isImplicitStackOperand

is wrong. This is not limited to GPRegs but needs to be able to store other kinds of registers that it gets as a string from XED.

[adsharma]

This is a great argument for actually using types. It wasn't obvious to me by reading the code what the type of reg should be. Of course, all other register types should be represented as enums IMO.

If a particular argument can be more than one register type, they should be defined as Union[GPreg, XMMReg, ...]

On Mon, Aug 2, 2021 at 7:25 AM Andreas Abel @.***> wrote:

And obvious downsides in this case are that it reduces readability, frequently calling "all(cls)" likely reduces performance, and it makes the code completely inconsistent if only general-purpose registers are represented as enums but other register types are not.

Your modification

• def init(self, reg, isImplicitStackOperand=False):
• def init(self, reg: GPRegs, isImplicitStackOperand=False): self.reg = reg self.isImplicitStackOperand = isImplicitStackOperand

is wrong. This is not limited to GPRegs but needs to be able to store other kinds of registers that it gets as a string from XED.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/andreas-abel/uiCA/pull/2#issuecomment-891071471, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFA2A5ICQZL2PJYLWX3HXLT22TEBANCNFSM5BJTMY5A .

[adsharma]

And if perf of all() is a concern, you could add a @lru_cache decorator so it would be computed once.

On Mon, Aug 2, 2021 at 9:12 AM Arun Sharma @.***> wrote:

This is a great argument for actually using types. It wasn't obvious to me by reading the code what the type of reg should be. Of course, all other register types should be represented as enums IMO.

If a particular argument can be more than one register type, they should be defined as Union[GPreg, XMMReg, ...]

On Mon, Aug 2, 2021 at 7:25 AM Andreas Abel @.***> wrote:

And obvious downsides in this case are that it reduces readability, frequently calling "all(cls)" likely reduces performance, and it makes the code completely inconsistent if only general-purpose registers are represented as enums but other register types are not.

Your modification

• def init(self, reg, isImplicitStackOperand=False):
• def init(self, reg: GPRegs, isImplicitStackOperand=False): self.reg = reg self.isImplicitStackOperand = isImplicitStackOperand

is wrong. This is not limited to GPRegs but needs to be able to store other kinds of registers that it gets as a string from XED.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/andreas-abel/uiCA/pull/2#issuecomment-891071471, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFA2A5ICQZL2PJYLWX3HXLT22TEBANCNFSM5BJTMY5A .

[andreas-abel]

This is a great argument for actually using types. It wasn't obvious to me by reading the code what the type of reg should be.

The type of reg should be string. This way, all the registers returned by XED (including ones that I am not aware of and ones that will be added in the future) can be handled.

Of course, all other register types should be represented as enums IMO.

Representing all register types as enums would require a lot of work and provide only comparably little benefit. There are many other things that have a much higher priority right now.

If you would like to contribute by adding new microarchitectural models, or by improving the existing ones, or by fixing actual bugs (that lead to a crash or incorrect results), or by adding new functionality, I will be happy to review your pull requests. I will not review pull requests that only make minor cosmetic changes.