*/+ fix comments & test messages, *add CompilerDevelopment.md*

[meisl]

Hi @irmen,

big thumbs up for 9bd3a67 ! This makes #49 actually useful :D

This PR is mostly a test for working on the new branch (I, for one, had some "fights" with git...).

But it also contains the plan that I promised, which outlines what I've done so far in my fork basically, only thought-over and reordered.
It is explicitly meant for discussion, even though written in a style that might not sound like that. Please don't be offended, it's just so to make it more concise. So again, I consider it work in progress, I'm totally open for discussion on every point.

[irmen]

looks good, there's a lot of good ideas in that document. I haven't studied it thoroughly yet that will have to wait

[meisl]

Thanks & sure, take your time.

I will, however, start integrating now, because I'm beginning to forget what I've done, and why.
About steps 1 and 2 at least I am pretty sure, but as said: totally open, if necessary I'm willing to do it over yet again.

I'll branch off of v7.1 for that, but you can of course choose where - and if at all - it'll go.

[meisl]

Update: new step in between the former 1) and 2), regarding ParseError (new), deriving ParsingFailedError (current, to be replaced eventually).

Even though I didn't really forget it, I now realized (as I'm integrating) that it's worth mentioning and discussing. It is a prerequisite for the following steps. To be precise: for testing them properly. And to have the successive commits be reasonable and comprehensible.

[irmen]

What would this give us? A char literal is equivalent to the (encoded) byte value of that character.

[meisl]

So that we don't need IStringEncoding for parsing, i.e. separation of concerns. One may ask the reverse: why would you not have it?

Note that I do NOT introduce Datatype.CHAR, I'm only moving this step into Compiler.processAst, thereby making it explicit and saving a lot of parameters (lowering the mental burden, not for perfomance), and, not least, removing this dependency.

It is in fact the single most effective step, and it took me a week or more to come up with (I did attempt to introduce DataType.CHAR, but that's something else and I'm glad to have found a way of not having to do this yet).

[irmen]

I've corrected the name of that misspelled file Antr->Antlr

[meisl]

Ok. But keep in mind that we should be very careful with renamings (particularly of files), as it complicates integration. The more so as long as there aren't enough tests.
The one exception in the plan, renaming the generated parser, also serves to definitely find all outside usages.

[irmen]

I realize this, so please take the opportunity to rename this file in your branch as well. It was just the file rename, there were no other dependencies on the particular file name.

[meisl]

Sure.

[meisl]

@irmen, I am realizing that I am unable to synthesize a chain of commits that reflects the order of steps in the plan.

I really tried, but the very task at hand is to disentangle things. So at first they are entangled, and I kind of need everything at once to "cut through" (mostly steps 1, 2 and 3).
But that isn't any more comprehensible than how I did it at first (rather on the contrary). Actually, now with a written-down plan, it is pretty easy to identify/classify the original commits.

So, what to do?

• I updated the .md re the file rename plus now saying "Steps to take, in conceptual (!) order".
• I need you to look at the commits. I could make another PR (draft) or add them here, you decide.
• I would add a few commits at a time, then comment on it, relating them to the plan.
• We can leave off steps 8 (refactor AST nodes) and 9 (rethink IStringEncoding), for later.
• What's missing so far is 6, 7 and 9; with 8 I started (but 8 and 9 can be left out, as said).

[meisl]

I've packaged together testability_steps_1_2_3 as a branch in my fork.

It's 15 commits (incl. 1 trivial file rename and 2 pure additions of TODOs), and ready either to be added here or for a new PR draft, whichever you prefer.
It adds 16 tests in compilerAst (from 9 to 25). However 3 of the new tests in compiler (-> your 9bd3a67) are failing. This is due to Module.isLibrary which we should discuss (I basically know how to fix it but need your judgement)
I think right after these 15 commits is a good point to go into details and decide how to move on from there.

Btw.: I've checked now: this would indeed resolve #46 (but ModuleImporter of course still needs to be moved and re-thought).

[irmen]

As a result of scrutinizing due to your questions about it, a7736d8 has now removed Module.IsLibraryModule variable and moved it to a function that is smart enough to figure it out by itself from the source path.
This may well fix your problems in the unit tests?

I hope to have more time next weekend to actually look into your commits, in testability_steps_1_2_3 .

[meisl]

Great, thanks for that and also for kindly + comprehensively answering my questions in #51!
That's one major thing out of the way, which I did not know how I would (=were allowed to) go about.

You see, I deemed the point where testability_steps_1_2_3 is at right now (3ea2cbd) a good one to step in because

• a) isLibrary is (therein) broken/unclear (as demonstrated by the new tests with examples + your making them actually useful!)
• b) Node.position and Module.source are (still) somewhat broken

My main question to you at this point would have been whether you would leave it as such and continue with step 4, or rather have it fixed - even with temporary hacks (be prepared) - and have tests added for it before moving on.
I did proceed working on the latter in the meantime, and with your a7736d8 I'm sure now this is the way to go. I still have to integrate it into my branch, but from glancing over it I don't expect any major problems.

I think it is best if I open another PR for the actual stuff, so we have this one here for the more general discussion.
In any case, with an actual PR we can use github's review features, which should make it at little less effort for you to look into it.
I'll stick to what I suggested above, and make the PR piece wise, that is with comments/hints inserted at appropriate points.
The first such point will be right after integrating your a7736d8 into testability_steps_1_2_3 since I do of course not want to open a PR that couldn't be merged to begin with. I'll make it a draft nevertheless, because well, that's what it is.
Then I will detail the problems mentioned above plus of course that they're partially solved by your a7736d8, as I'm pretty sure. Next bunch after that will be plus what I've done myself regarding the "fixing up after 1,2,3", let's call it.

It'll take some time, I need to try out a few strategies of merging locally before putting it onto github.

[meisl]

Okay, I've done it again (rebasing, not fun at all), and opened the PR draft. That's now the reference, not my fork.

Quite some problems - if not all - that I mentioned above aren't there anymore, and it's now 14 commits (net).

[irmen]

@meisl is this PR still relevant or is everything in here also already in testing123 in your clone? If so we can close this one and just concentrate on getting that other one reviewed and merged :)