softdevteam / ykrustc

Yorick Meta-tracer

Other

6 stars 4 forks source link

Add binary operation lowerings. #97

Closed vext01 closed 4 years ago

vext01 commented 4 years ago

I think this is ready to go.

ltratt commented 4 years ago

As a general point (not really for this PR), is it a bit verbose if so many functions start with lower_? In my experience, a shorter prefix is more sustainable in the long term (e.g. l_).

vext01 commented 4 years ago

I prefer lower_ I'm afraid.

vext01 commented 4 years ago

Let me expand on this, as I do feel somewhat strongly.

I can't count the number of times I've had to ask what the meaning of a single- (or two-) letter prefix is. I'm in the position where I can easily ask what they mean, but not everyone has that luxury. It's better if the code speaks for itself, leaving no ambiguity.
It's not the Rust way and the precedent in the compiler is to use descriptive names, e.g. codegen_block instead of c_block.

ltratt commented 4 years ago

We've agree offline to leave the naming convention as-is in this repository.

ptersilie commented 4 years ago

Looks fine. Please squash.

As for the lowering_ discussion, I'm leaning towards Edd here. It's still short enough to not offend my eyes. If the function name was super longer then that would need another discussion.

vext01 commented 4 years ago

splat.

ptersilie commented 4 years ago

bors r+

bors[bot] commented 4 years ago

Build succeeded

buildbot/ykrustc-buildbot-build-script-hwt