search

sorear / smetamath-rs

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

Little fixes #38

Closed david-a-wheeler closed 3 years ago

david-a-wheeler commented 4 years ago

@sorear - this is a small set of trivial changes that eliminate most compiler warnings. This is also a gentle way for me to start learning your code base.

sorear commented 4 years ago

If the old code doesn't build that is aggravating - Rust and Cargo repeatedly and in no uncertain terms promised us stability.

If this is just about suppressing warnings, I think that should be done with #![allow] directives unless there is a specific functional bug that is fixed by the change. (The Debian OpenSSL fiasco is still fresh in my mind.)

david-a-wheeler commented 4 years ago

It builds, but this is not about suppressing warnings, this is about eliminating them. For example, is anyone using smetamath using a Rust implementation where ? is not implemented?

I'm quite familiar with the OpenSSL Debian problem, but the problem there was a failure to engage the openssl folks. Also, that was cryptokey handling, which is always very sensitive, this code is not handling crypto keys.

david-a-wheeler commented 4 years ago

I think it's important to look to the warnings, and eliminate them where there are reasonable. In this case, I think they make the code much easier to read.

Old: Correct rust tutorials don't even mention try!, they only teach about ?.

Correction: They mention try!, while explaining why to use "?" instead.

david-a-wheeler commented 4 years ago

Note: The "?" was added in the 2018 version. If you want to support the 2015 version, it's fine to add "allow" directives to prevent that warning... but I think it should also be documented that the goal is to support the 2015 & 2018 versions of Rust. Ideally, automatically verify there are no warnings for both versions and that the tests all pass.

david-a-wheeler commented 3 years ago

This change conflicts with the PR that Mario submitted a few years ago. So I'm going to withdraw this proposed change, re-fix the code, and apply that commit on my friendly fork instead. I still think that applying these kinds of changes both (a) eliminates the warnings and (b) makes the resulting code easier to read.

You're very welcome to apply that resulting change to smetmath-rs if you want to.