5 ms·

>> remove `sym_equal` entirely and replace all usages of it with `rb_obj_equal`. ...it must have the same identity as `rb_obj_equal` That's exactly what #defin

>> remove `sym_equal` entirely and replace all usages of it with `rb_obj_equal`. ...it must have the same identity as `rb_obj_equal` That's exactly what #define does. It replaces all usages and gives the token the same identity as the other token. As a C programmer first and foremost, it looks reasonable to me.

munificent 12y ago
Yes, I understand the semantics of #define. My point is that someone seeing a reference to sym_equal may not realize that (1) it's identical (at the C level) to rb_obj_equal and that (2) that fact has important performance ramifications. First class functions are super useful in C, but I think it's pretty rare to rely on their identity at runtime. If you are doing that, also aliasing the same function by another name seems like a recipe for confusion.
- jenrzzz 12y ago
  It's an optimization hack. By nature, they aren't that nice to look at. `sym_equal` only makes 2 appearances in the whole codebase (to be bound to `Symbol#==` and `Symbol#===` in string.c), so there's not a whole lot of room for confusion. Also, this is Ruby core! Smart people do what works and makes sense to them and don't spend a lot of time worrying about how clear it is to us unwashed masses.
  - wmt 12y ago
    Come, that's just plain wrong to say that the code quality cannot be questioned because it's an optimization and because it's made by "smart people" and it "makes sense to them". No code is sacred. It's perfectly valid to question using a macro to rename a function instead of just renaming calls, especially if the renamed function is only called twice. It also would've been more pragmatic to mention in the code that rb_obj_equal is being used due to an optimization in opt_eq_func, as it's not obvious. There might be reasons for using a macro in this case, like wanting to underline the fact that symbols are being compared, but I strongly suspect the author was just focused on changing the implementation of sym_equal to be better, and didn't think of the option of throwing it away.
    - jenrzzz 12y ago
      I didn't say it couldn't be questioned. I was just explaining some possible reasons why the patch was so esoteric, and pointing out that things that seem straightforward to MRI hackers often aren't to people who don't dig through Ruby internals every day.
- mtdewcmu 12y ago
  >> My point is that someone seeing a reference to sym_equal may not realize that (1) it's identical (at the C level) to rb_obj_equal and that (2) that fact has important performance ramifications. Exuberant ctags will find the #define that goes with sym_equal and make it quick to locate. Upon seeing that sym_equal is #define'd to rb_obj_equal, it will be clear that they're identical at the C level. It may not be obvious why it's a #define, but a cautious programmer would be on alert that changing it back to a separate function would likely have some kind of implications. Perhaps the ideal solution would be a large refactor to make the whole system as self-explanatory and surprise-free as possible. While waiting for that, this #define looks like a reasonable and low-risk bug-fix.

munificent 12y ago

Yes, I understand the semantics of #define. My point is that someone seeing a reference to sym_equal may not realize that (1) it's identical (at the C level) to rb_obj_equal and that (2) that fact has important performance ramifications. First class functions are super useful in C, but I think it's pretty rare to rely on their identity at runtime. If you are doing that, also aliasing the same function by another name seems like a recipe for confusion.

jenrzzz 12y ago
It's an optimization hack. By nature, they aren't that nice to look at. `sym_equal` only makes 2 appearances in the whole codebase (to be bound to `Symbol#==` and `Symbol#===` in string.c), so there's not a whole lot of room for confusion. Also, this is Ruby core! Smart people do what works and makes sense to them and don't spend a lot of time worrying about how clear it is to us unwashed masses.
- wmt 12y ago
  Come, that's just plain wrong to say that the code quality cannot be questioned because it's an optimization and because it's made by "smart people" and it "makes sense to them". No code is sacred. It's perfectly valid to question using a macro to rename a function instead of just renaming calls, especially if the renamed function is only called twice. It also would've been more pragmatic to mention in the code that rb_obj_equal is being used due to an optimization in opt_eq_func, as it's not obvious. There might be reasons for using a macro in this case, like wanting to underline the fact that symbols are being compared, but I strongly suspect the author was just focused on changing the implementation of sym_equal to be better, and didn't think of the option of throwing it away.
  - jenrzzz 12y ago
    I didn't say it couldn't be questioned. I was just explaining some possible reasons why the patch was so esoteric, and pointing out that things that seem straightforward to MRI hackers often aren't to people who don't dig through Ruby internals every day.
mtdewcmu 12y ago
>> My point is that someone seeing a reference to sym_equal may not realize that (1) it's identical (at the C level) to rb_obj_equal and that (2) that fact has important performance ramifications. Exuberant ctags will find the #define that goes with sym_equal and make it quick to locate. Upon seeing that sym_equal is #define'd to rb_obj_equal, it will be clear that they're identical at the C level. It may not be obvious why it's a #define, but a cautious programmer would be on alert that changing it back to a separate function would likely have some kind of implications. Perhaps the ideal solution would be a large refactor to make the whole system as self-explanatory and surprise-free as possible. While waiting for that, this #define looks like a reasonable and low-risk bug-fix.