david-a-wheeler / mmverify.py

Metamath verifier in Python
MIT License
35 stars 10 forks source link

Some unused variables #4

Closed benjub closed 2 years ago

benjub commented 2 years ago
  1. As noted in #1, stat_type is unused. It is initialized to the type of the statement whose proof is being verified, so in (i)set.mm it will always be "|-" (but it could take multiple values in a given database). The verification of a proof does not involve the typecode of the statement by itself. I think that variable could be removed.

  2. As mentioned in #3, the variable frame in the function make_assertion is unused. It makes sense since the latest frame (the "present scope") is not particularly relevant to the function make_statement: what is relevant is the "union of the framestack", which is why in that function, self only appears as in self. I think that variable could be removed.

  3. The function verify has an unused argument stat_label. The function verify (stat, proof) verifies that proof is a correct proof of stat, and the label of the statement has nothing to do with it. I think that argument could be removed.

  4. The use of autopep8 formatting changes indentations slightly. Should this be adopted ?

  5. In the three functions lookup_c/v/d, the reversed seems to be superfluous ? In lookup_e/f, it seems correct to have reversed since this will return the label of the latest active e/f hypothesis with given statement. Similarly correct for both uses in make_assertion.

(letting @raphlinus and @sctfn know)

benjub commented 2 years ago

It seems also that Line 67 is superfluous, that is, the bloc

            if tok == None: return None
            if tok == '$(':
                while tok != '$)':
                    tok = self.read()
            else:
                return tok

can be replaced by

            if tok == '$(':
                while tok != '$)':
                    tok = self.read()
            else:
                return tok

Also: typos within comments: Incusion --> Inclusion have be length --> have length

benjub commented 2 years ago

I modified the code on my local fork (https://github.com/benjub/mmverify.py/tree/streamline). In particular:

As a result, performance (speed) improved by 6%--10% on set.mm and iset.mm.

@david-a-wheeler : should I do a PR ?

david-a-wheeler commented 2 years ago

Definitely do a PR. If it seems likely the variable will be used later, documenting how to get it later in a comment might be appropriate.

benjub commented 2 years ago

@david-a-wheeler : or maybe you want to go directly for the 4 times faster (!!!) version: https://github.com/benjub/mmverify.py/tree/faster ? (which still needs some checking and testing)

benjub commented 2 years ago

Closing since fixed in #7.

david-a-wheeler commented 2 years ago

Speed is good, but correctness is more important :-).