search

gerryjackson / forth2012-test-suite

Test programs for Forth 2012 and ANS Forth
72 stars 15 forks source link

Addition of utilities.fth #4

Closed gerryjackson closed 8 years ago

gerryjackson commented 8 years ago

Provides definitions for use in the optional word set test programs

steverpalmer commented 8 years ago

These notes are super-critical nit-picking. There is nothing in this branch which is anywhere close to a show-stopper. Nevertheless, my notes follow:

  1. The utilities.fth file now provides a conditional definition for \, but this has already been used in runtests.fth, tester.fr, ttester.fs, errorreport.fth and even core.fr. I agree with the idea of trying to reduce the dependencies on words outside of the core, but I think that the change required here is bigger than what has been proposed. I wonder whether, for now, the test suite should make the existence of \ a dependency on the files that use it and raise an issue that the test suite has a dependency on the core extension . I know that this is pushing off the problem to another time, but I struggle at this stage to see a way in which the conditional definition of \ in the utilitites.fth file will used.
  2. In runtests.fth, the utilitites.fth has been included after the errorreport.fth include. Today, errorreport.fth does not depend on utilities.fth (ignoring the \ dependency mentioned above), but I can imagine a time when it might benefit from core extension words. On the other hand, I can't imagine a situation when the utilities.fth should depend on errorreport.fth words. Therefore, I'd suggest swapping the order of these two includes in runtests.fth.
  3. The addition of blocktest.fth is imminent (honest :-)), but is pre-empted in both runtests.fth and errorreport.fth. To make the blocktest branch complete and self-consistent, it might be best to leave the changes to runtests.fth and errorreport.fth to the changeset for the addition of blocktest.
  4. In utilitities.fth is a definition of BUMP which looks like the word /STRING. Is there a reason for giving it a different name? It could be given a conditional definition independent of PARSE, like 2CONSTANT below. (If it walks like a duck and quacks like a duck ... :-))
  5. It would be good to include a few tests of the [?DEF], \? words. As we've discussed by email, I think that it is important that we know that the conditional defined words like NIP, TUCK and 2CONSTANT do not always get defined and then potentially hide a faulty built-in definition of these words.
  6. In core.fr, there is a definition of S= (line 819), and in the utilitities.fth there is an equivalent definition of STR=. As far as I can see, the two words will behave the same, but they have distinct implementations. This is partly because the utilities.fth definition makes use of core extensions like TRUE, but there are other distinctions which don't seem to add value. (Also, the indentation of S= in core looks odd to me.) I'd suggest:
    • pick one name and use it consistently, and make the definition in utilities.fth conditional.
    • pick one implementation and use it in both places.
    • (I know that this breaks the rule of "say it only once", but there is good reason to break this rule in this case.)
gerryjackson commented 8 years ago

Looking at each of your points

  1. This is the same problem that tester.fr has where many words are used before being tested. That was why I wrote the Pre-Hayes test program (need a better name) which does test for \ and conditionally defines it (inadequately for blocks). So if and when we introduce that the problem will be addressed. We could always specify it be run before tester.fr is loaded and, having proved it works users can omit it if they need to. As I wrote before I'd rather leave that early test until the next version so we should raise an issue that can be closed later. I see what you mean about the conditional definition of \ so we may well delete it for now. You've probably seen the issue I raised about runtests.fth anyway. We could always replace use of \ with ( ... ) in runtests.fth but given it's in tester.fr I suggest we don't bother.
  2. Agreed. I'm not sure why I put it there, I'll swap them
  3. OK I'll remove them for now
  4. Yes, well I originally used /STRING and then replaced it with BUMP when I realised /STRING is in the String word set. If we define standard words like /STRING if they are not in a system then the utility definitions get tested later on which is silly. I think I realised that with /STRING and then didn't follow it through. I'm now thinking we should do away with these conditional definitions and just define our own equivalents, perhaps prefix them with T- so that we define T-PARSE T-/STRING where T stands for tester, I had thought U for utility but U looks like unsigned.
  5. If we follow my suggestion in 4. this goes away.
  6. Yes I knew about S= in core.fr but didn't use it because I didn't want any other test programs to depend on definitions in core.fr. This is because I often comment out all the early test programs so that the one I'm working on runs immediately and I'm assuming that other users would do the same. That's why I don't use the core.fr and MAX_INT etc which I used to use. I've deliberately left core.fr and tester.fr alone, as much as possible, because of the copyright statements. I probably worry too much. Conditionally defining S= in utilities sounds best to me. I'll do that.