search

sorear / smetamath-rs

sorear's Metamath system engine - version 3 Rust
Apache License 2.0
22 stars 6 forks source link

random minor style changes #19

Closed digama0 closed 8 years ago

digama0 commented 8 years ago

Just a few minor Rusty things, nothing which should change the operation of anything.

sorear commented 8 years ago

I hate to say this, but I'm not crazy about most of these.

digama0 commented 8 years ago

This is mostly just me coming to terms with the implementation and making sure that I can in fact make changes without catastrophically breaking everything. I'm not really attached to any of these changes, feel free to reject them, it was just stuff I noticed as I read. However, I would recommend at least accepting the Option<Diagnostic> -> Result<()> change even if it takes longer, because it better represents the actual purpose of the code (and also allows usage of try! for early error returns). Also, try not to spend so much time on micro-optimization going forward. I would just watch the benchmark speed and maybe take action if it slips significantly.

digama0 commented 8 years ago

I rebased the "scopeck modifications" to separate out the OO-ification of ScopeState and InchoateFrame from the other code style things so that you can see better what changed.

sorear commented 8 years ago

Many of the changes here I'm fine with. The ones that I'm not are the ones that either revert a deliberate measured optimization (most of which you've already reversed), or the ones that in my perception start turning the code into Java or Haskell.

My current sensibilities with this stuff are largely Go-inflected:

  1. Not all functions need to be methods and not all structs need to be "objects"
  2. Loop nests are almost always clearer and better than iterator-fu
  3. It is better to minimize the complexity and punctuation count per line, than to minimize the total line count
sorear commented 8 years ago

Unsure what to do with this PR. I don't want to accept it as is, it'll take me a long time to separate out the stuff I want from the stuff I don't; I'm not eager to spend that time now, because I want to do other things with this code; things which will make the PR unmergable due to the sheer number of diffs in this code…

digama0 commented 8 years ago

Just list the things that you don't want, and I'll take it out. From your last comment, I take it that you are still against the objectification of VerifyState, ScopeState, and InchoateFrame. I maintain that it is not "going down the dark path" unless it is combined with the reduction and merging of structs into larger ones. What I have done here is purely a meaning-preserving and valid rust transformation, which rust explicitly has syntax sugar to support. If I still can't convince you, say the word and I'll take it out. Better yet, I'll take them out of this and add them to another PR so you have more individual control.

Not sure whether to take 2. as a general proscription against all "iterator-fu", which would be a great shame. You can of course make these expressions more spread out by adding more newlines and/or comments of explanation, assuming the formatter lets you.

You've already been quite helpful in commenting the things you like and dislike; but since some of the "dislike" comments are not clear rejections, it's not clear to me what is blocking acceptance. Just add "reject" to the diffs you don't want in the PR, and I'll do all the cleanup work. (I'm keeping the commits separate exactly because the overall diff is a mess.)

sorear commented 8 years ago

I take it that you are still against the objectification of VerifyState, ScopeState, and InchoateFrame.

Moderately, which is the problem. If I were strongly against it I'd have no problem saying so, but I'm stuck waffling instead :(

Trouble is that style things are usually opinions and I can't back up most of them, because opinions.

Not sure whether to take 2. as a general proscription against all "iterator-fu", which would be a great shame.

I don't object to iterators, I object to changes that "feel" like replacing newlines with punctuation. I'm OK with higher-order functions that do substantially more than a for-loop, but map often reads to me as "for loops waste space on the right-hand side of my screen, my code needs to be justified like a newspaper article". Again, opinions.

sorear commented 8 years ago

Also I come from a place where I regard large unsolicited style-changing patches as quite rude, and I've lost much of the last week's worth of smetamath to waffling over whether I should just close this … smaller changes would go over better.

sorear commented 8 years ago

Regretfully I must ask you to come back with smaller patches that I can review without hitting burnout on 1/4 of the way through.