A "code review" of fouric's Mercury project!

mercury-review.md

Mego's code review of Mercury 2019-4-30

I'll read through each file and write verbose live comments in hopes to provide some insight as to what might be going on in somebody's head while reading through it! (Although I'm just one person, and I don't read through others' code that much, and I've never done a "code review" like this before, anddddd I'm sure you know more CL than I do, so with all that in mind, of course take it all with a grain of salt XD)

Also keep in mind that this is very much a "speak before you think" scenario, in order to show you what's going on in my head, so much of what I say will be ininformed at first, and then gradually become more informed. You might want to read the conclusion first, to see what my final and more refined thougths are before delving into my completely unrefined thoughts! :D

After reading each file, I'll write a short summary of my thoughts after having read the whole thing. And finally, after I've read through all of the code, I'll write a short summary of everything at the very end, so you can simply skip to the end to get the gist! (It also gives me a chance to give you my proper thoughts after having read and thought about it a bit.)

Also I'm not entirely sure what the best order to read through the source files might be, so I'll start with mercury.lisp and move through there as what feels most intuitive. Keep in mind also that this arbitrary order that I read through the files in will have a significant effect on my understanding. (Which is an exact issue we talked about in one of our calls before! :D) Many questions I have might be simply because I should have read a different file first. (In retrospect, I realize I probably could have taken inspiration from the order the files are written in in mercury.asd although, I'm still not sure whether I should be reading from top to bottom or bottom to top, what do you think?)

If you feel like it, I wouldn't mind if you gave me a review of my review :P Pointers on how to be more helpful, since I've never done this before. For example, I'm not entirely sure whether this level of verbosity is valuable and if it wouldn't be more efficient simply to write my final thoughts after looking over everything.

mercury.lisp

It doesn't seem entirely obvious to me what *symbolic-source* is at first glance. But after a short pondering my intuition tells me that this is the code loaded into the editor to be edited. It makes sense since this is a structure editor, there is no need to store any raw text in memory!

However, if that's the case, I'm wondering what *ast* is. Is *ast* actually the currently loaded code to be edited? And *symbolic-source* just the example code?

I'm wondering what *tokens* is. At this point it's sounding vaguely like some explicit parsing is going on somewhere, haha. Actually, now that I think ahead a little, I realize that the text does have to be explicitely rendered, so maybe these are to do with that!

The *settings* parameter seems intuitive, thanks to the comments!

main seems pretty straightforward, it looks like it pre-renders the code, sets up the charms window and enters the main loop.

Oh hey, on-global-update is a really cool function! (If it does what it looks like it does, haha.) That way you don't ever have to explicitly update things after any operation that changes something (which in my experience is really unpleasant and bug-prone XD)

So anyway, it looks like it draws the screen, reads input, exits the program if some quit value was read, exits the program and quits slime if some dev-quit value was read, and otherwise just loops. Seems pretty straightforward!

Summary

Everything seems pretty clear to me, the only thing I'm slightly unsure about are the *tokens*, *ast*, and *symbolic-source* parameters. My suspicion at this point in time is that *tokens* holds the "rendered" text (given to charms for coloring, etc) and *ast* is the list structure that it renders from. I know *symbolic-source* is defined here as what if I remember correctly shows up on the screen by default when you first launch Mercury, so I'm not sure if this is just a "placeholder" code, or if it's the actual edittable code as held in memory. If the latter, I'm wondering what *ast* is for, since it seems to me that *symbolic-source* already is a parsed AST. I'm sure it'll become clear soon enough though, haha.

render.lisp

node-depth is very simple and clear.

The generic function render is pretty straightforward. Maybe it's because I just woke up, but it took me slightly longer to feel like I know what in-selection was for just by looking at it. (Like, 10 seconds, so not really a big deal XD)

This might just be a lack of CL knowledge on my part actually, but is some sort of method combination going on here? In light of that, skimming briefly just to see what types of nodes render is defined for, the only couple of things that aren't immediately obvious to me are these:

1. Is container a list? Are there other types of containers? Is the internal representation of the AST not guaranteed to be parsed as lists specifically?
2. If I understand correctly, a container would be a case of a form, in which case I feel slightly confused by the method for container being defined first. But take that with a grain of salt! I'm not even sure container is what I think it is yet, lol. This isn't even that important anyways. XD
3. Why there is both a method for literal as well as a few types which seem to be to be cases of literals.

At first glance, I was wondering why there is an explicit check for whether the node is the selected form when there is already an in-selection parameter. But then I realized that there is a difference in being the selected node and being in the selection! And that the value of in-selection is probably passed down recursively to each nested node, so that makes sense!

Okay! So it looks like the container render method simply renders each element, flattens the list of tokens (I'm assuming at this point that's what they are returning haha), and adds a newline at the end.

Things I'm not entirely sure about at this point:

1. What the purpose of whole is in (whole node). (Maybe this is a hint that container isn't exactly what I think it is?)
2. Why the newline is wrapped in a list, if it's being flattened anyway. (I might not know exactly how alexandria's flatten works.)
3. I'm also not entirely sure at this point what tokens actually are. My best guess is that they are generically any sort of value that charms knows how to print.

(At this point I think it does make sense why the container render method might have been put before the form render method. If nothing else, it feels more comfortable to think through it in that order! I'm also realizing that maybe this has a lot to do with how method combination works, and again I don't entirely know how it works, lol.)

Woah! That's some very indented code! :D (By the way, I think I agree with you that wrapping should, ideally be handled by the editor and not manually by the user.)

Some side thoughts:

1. Do you think color themes should be configurable rather than colors being hardcoded? Or is it more of the case that these color names are idiomatically treated more "symbolic" than literal? I say that, because I know a lot of terminal color themes blatantly ignore the names of colors and just put whatever colors they want there, lol.
2. This is hardly a big deal at all, I'm just being overly nit-picky, lol. But I wonder if there is a way to simplify (in-selection (or in-selection (eq node *selected-form*)), particularly because its repeated in each method. But at the same time I can't really think of an obvious good way to do it. Since if it were a macro, it would be the kind of macro that specifically relies on "poor hygien" and I'm not really sure if that's a bad idea or not? What do you think? o:
3. I notice (if in-selection 'black-white 'white-black) is repeated as the alternative case in paren-color, maybe it could just be normal-color instead, since it's already calculated and this is already a let*?
4. Do you think the list of paren colors should be defined at the top of the file or something instead as a parameter, to shrink the code here a bit? My reasoning is that it has nothing to do with the logic, and it would be more concise semantically as a descriptive variable name.
5. This is just a small performance observation. I notice the node depth is calculated from scratch for every node. I imagine it would be faster to provide the node depth as an argument to each recursive render call. (Although, I'd be concerned about how that would affect readability. I'm also not entirely sure whether this would make a big enough difference in performance to be worth considering.) Alternatively, I wonder if the same performance gain could be had without sacrificing readability at all by simply adding a cache to node-depth? What do you think?

It took me a minute to understand name-color but it looks like it has something to do with highlighting a form in some way if the first element of it is selected?

Unfortunately I can't quite make heads or tails of rendered at all, lol. indented is kind of confusing to me as well.

Finally, the very last line seems to shed a bit of light on what tokens actually are, I think? If I have the right idea here, it might be more clear to have some sort of token "constructor" to make it more obvious that its a token being returned here and not just some list haha.

Oh! I see, so the color parameter is actually at some point a semantic symbol (normal, other-name), and then at a lower level becomes an actual charms color symbol (pink-white, pink-black, etc.). I wonder if there is a way to make this more evident? Or maybe its not actually important? Not sure. XD Oh! Okay, I realize now that the parameter is always a semantic color symbol, and there is a separate color being defined in the lower level methods for the charms color. I wonder if it wouldn't be beneficial to have separate names for these two different color values to prevent confusion?

Summary

1. I wonder if putting a cache on node-depth would be worth the performance benefits?
2. Would it make sense to condense (in-selection (or in-selection (eq node *selected-form*))) into a macro, despite being "poor hygiene"? Or is there a better way? Or is it simply not that important? Haha.
3. What do you think about defining the list of paren colors at the top of the file and replacing their occurence with a descriptive variable name?
4. What does (whole node) do?
5. Should colors be hard-coded like this, or moved into a color configuration structure of some sort?
6. While its toplevel purpose is fairly obvious, I can't quite read rendered (line 25) lol.
7. Ditto but to a lesser degree indented (line 45).
8. What do you think about having a token "constructor"? If for nothing else, purposes of readability? (Instead of returning lists explicitly.)
9. There are two different color values being passed around, what do you think about giving them separate names in order to avoid confusion?

ast.lisp

Aha! So here is where whole is defined. Although it still isn't obvious to me what it means.

This also explains that container is in fact not a list. That makes sense! I think I was accidentally assuming that form, literal, num, sym, and str were all builtin to CL, which now that I think about it, I'm not sure why I was assuming that. XD

Okay! So, skimming this, this answers my question about the difference in the globals *ast* and *symbolic-source*. It looks like *symbolic-source* is the code as raw lisp data, whereas *ast* is the code parsed into instances of the classes defined in this file. My intuition is that the purpose of this is primarily to provide backreferences to the parent nodes? (And if that's the case, I wonder if that need could be eliminated by simply changing the way node depth is calculated? I say this very naively, though, because there may very well may be other purposes for having this explicit AST representation that I'm not aware of yet!)

I'm guessing these *functions*, *macros*, and *special-forms* are for categorizing different keywords so that they can be parsed specially? I'm also guessing they are incomplete. I'm not sure how exactly they are to be used yet, but I imagine that will become clear.

parse-to-ast seems pretty straightforward! Simply converting from Lisp types to the corresponding AST representation as defined above.

Reading the alternative case, I think this clarifies as well the semantic difference in a list and a container. For example, the body of the entire file is a contaner but not a list.

Hmm, it looks like the form function creates a form instance given the whole and the parent, but since this constructor doesn't appear to be defined in this file, I'm not 100% sure. (Unless defclass actually implicitly creates this constructor and I don't realize that? Not sure lol.) Actually, I'm even more in doubt now that I realize this is being treated as a list here in dolist. But at the very least, it seems clear that the dolist serves to attach parents to each child in the the container.

Alrighty, so this may just be a lack of understanding on my part, but I feel like parse-to-ast is a bit muddy, and that it could be written in a simpler and more concise way. I'll include my thoughts in the summary!

Summary

1. It's not entirely obvious to me yet why this explicit AST representation is needed over raw lisp forms, aside from backreferences to parents.
2. It appears that the alternative case to already-wrapped? is only used as the top call to parse-to-ast. Do you think this would make more sense to simply have as a separate function? So a top-level parse-to-ast function that is this alternative case, which then calls a separate recursive function that does the main parsing logic.
3. It's not immediately obvious to me at this point what the need is for explicitly classifying function invocations, macro uses, and special forms.
4. The code for assigning parents is a bit confusing to me (assuming that's actually what it's doing here).

charms.lisp

draw is clearly about as simple as it gets, not sure why I'm even commenting on it XD

Okay, paint-tokens, let's read it. For each token, -- okay, hold up XD Do I not understand how cond works? I'm thrown off by the fact that it looks like there are three items in this first cond form?

Oh so it looks like these color pair symbols are for internal use, and are different from the names charms uses? Do you think it would be nicer to have this translation table defined elsewhere? Again my reasoning is that it has nothing to do with logic, and distracts from the readability of the logic.

Related to the previously mentioned idea about having a token constructor, do you think it might be helpful to simply have a token class? It looks like you are using symbols to represent color changes, but I don't think that would be immediately obvious to me had I not read through some of the other code already. I imagine having token subclasses for each type of token would make it much more apparent what each token means, for example a color-change token and a text token and a newline token.

Summary

1. The three-element cond form confuses me. lol (Could just be me lacking CL knowledge. I tried looking it up but nothing was obvious to me.)
2. What do you think about defining the color-pair lookup table elsewhere? (Reasoning: it has nothing to do with the logic and is otherwise distracting.)
3. What about having a token class with color-change-token, text-token, and newline-token subclasses in order to be more semantically concise?

input.lisp

Okay, cool! handle-input is very clear, a simple mapping of character inputs to handler functions that may or may not return a useful "return code" that's used by the main loop (specifically for quitting).

Oh! I see the undo and redo functions explicitly re-render the AST (and, in retrospect, many of these functions do). Is there a reason you haven't used your on-global-update function for this instead?

Hmm, why is render-from-global specifically setting the selection to the second argument of the first form? Ohhh, maybe it's because render-from-global is only used when the source code is initially parsed, and it's just setting up a selection to start with (probably somewhat arbitrary, for testing, I imagine).

Also, I'm realizing now another reason why having a reference to the parent is important aside from simply finding the node depth, haha.

Summary

1. Is there a reason not to use on-global-update to re-render the AST when it changes?
2. Idea: When moving the selection around, instead of explicitely finding the position within the AST, and manually calculating which node to select from that, what if node objects themselves simply had attributes for next-sibling and previous-sibling? They could be implemented as reader methods. Maybe then you could condense all of the select-x functions down to a single function that takes a direction argument. I'm not sure whether all of this would be valuable or just redundant though. At the very least it might clear up some of the code duplication here and separate the concerns a bit more. What do you think?

xdo.lisp

This is a cool undo/redo system! :D

Okay I like the idea of xdo-change. One thing that isn't immediately clear to me is what kind of value old and new are. Are they other xdo-change instances?

make-change is a little bit confusing to me, but it looks like somehow or another changes made to slots and bindings are treated explicitly? Oh okay, yeah, the apply-change methods clear that up.

One thing that wasn't immediately obvious was that apply-change means "move forward in time" and reverse-change means "move backward in time." Although... that just might be because I'm a bit sleepy, because they really do seem like very appropriate names. XD

It looks like xdo-commit is defined twice, lol, accident?

Naming is sometimes a tricky thing. XD I can see the difference in "make commit" and "apply commit" although at first reading, "make commit" sounds like it could mean what "apply commit" means. I think this is just a nitpicky thing with no obvious solution though. Afterall, when seen side by side I think it's fairly obvious that "make commit" is a constructor and "apply commit" is an action.

So yeah! I think it might be helpful to include a brief summary of the concepts used in xdo at the top. For example explain commits and changes, and the undo and redo stacks. After reading through I think it's pretty clear, but I did feel confused at first.

Summary

1. Maybe provide a brief explanation at the top, especially of commits and changes. It especially wasn't obvious to me at first glance that old and new are slot/binding values.
2. Duplicate xdo-commit definition! :D

Concluding thoughts

Cool project and code! :D I think for the most part it's pretty readable, with a few exceptions in particular. I'm going to go over each of my summaries and put my final thoughts on what I think are the most important points here:

1. It wasn't immediately obvious to me the nature of *symbolic-source*, *ast*, and *tokens* at first, but became clear to me later on as I realized you have a distinction between the source form and the way you are representing the AST internally. I did suspect that *tokens* was used for rendering with charms, but I didn't realize how this was being accomplished at first.
2. I wasn't sure at first why you have an explicit AST representation instead of simply using lists. Later on I realized it primarily seems to be because of the need for having a reference to parent nodes (most notibly for navigation/selection, and additionally for calculating node depth).
3. I was confused about how tokens are represented throughout the code. I think it might help clear up the semantics of tokens if there was a token class with subclasses for each type of token.
4. In the two cases where you have inlined lists of colors (line 18 and 19 in render.lisp and starting on line 16 in charms.lisp), I thought it might help readability to move that data outside of the function. My general reasoning is that in these cases, it has nothing to do with the logic, and would be take up less space and be less distracting as descriptive variable names.
5. The rendering and indentation logic in render.lisp are I think the most confusing parts of the code, haha (lines 25 and 45).
6. To my understanding, there are two kinds of colors in render.lisp, one for "color class" (normal, other-name) and one for actual colors. I thought it might be less confusing to give them different names.
7. I thought it might be more readable to separate the two parts of the if form in parse-to-ast into two separate functions instead of having what seems to be effectively a switch for distinguishing the top-level call.
8. I had trouble following the code for assigning parents to children in parse-to-ast.
9. I was very confused by what seems to be a cond form with three elements in charms.lisp XD
10. Is there a reason for not using on-global-update on *ast* in input.lisp?
11. I don't think it's particularly an issue, but I feel like the selection navigation functions in input.lisp feel a bit muddy and could be condensed a lot. I gave my ideas in the summary above!
12. I think xdo.lisp is pretty clear, although going in, the concepts of "changes" and "commits" wasn't 100% obvious to me at first! As well as how changes actually are implemented was a confusing until I realized how it actually works.