[PATCH] D137085: [LLVM][TableGen] Add first tutorial notebook

[David Spickett via Phabricator via llvm-commits]

DavidSpickett added inline comments.


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_tutorial_part_1.ipynb:15
+   "metadata": {},
+   "source": [
+    "**Note:** The content in this notebook is adapted from https://llvm.org/docs/TableGen/index.html.\n",
----------------
Eymay wrote:
> I get the reasoning behind `%reset` but there is a risk that a complete beginner may  miss this sentence and think of it an integral part of TableGen.
> If it could be greyed with a syntax highligher that could help. Or a different logic for clearing cache like a %global can be used. I found it was a little annoying seeing it in every cell block.
We have conflicting use cases going on.

For a tutorial where each snippet is standalone, I agree that making `%reset` the default would be correct.
For a tutorial where you build up a large example by combining snippets as you go, you would want a default of `%global` (as you put it).
For prototyping something bit by bit, the same applies.

Most Jupyter kernels default to global cells, because they're writing to an interpreter process underneath. So I've stuck with that tradition.

We could add a magic to configure it. I think "noreset" would be a better name than "global" given how the kernel works, but you get the idea.
```
%cellsreset on
```
Then if the default is not what you want for a specific cell you can do `%reset`/`%noreset` in that one cell.
I'd argue the default for the kernel should be `%reset` then for these tutorials we change that to `%noreset`.

TLDR: Yes the line noise is annoying, I will look at adding a configurable default for it.


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_tutorial_part_1.ipynb:77
+   "id": "58f4b3e8",
+   "metadata": {},
+   "source": [
----------------
Eymay wrote:
> Embedding links could be better
I'm taking embedding to mean linking words rather than pasting the literal link in. I've updated the links to do that.


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_tutorial_part_1.ipynb:154
+  },
+  {
+   "cell_type": "code",
----------------
Eymay wrote:
> Eymay wrote:
> > first let can be enclosed as `let`
> It is a little difficult to track, more descriptive names instead of X,Y,Z can maybe help the reader. 
> 
> Another option is to keep the structure consistent with the previous code blocks so it can be more familiar.
> For example, X can be repeated as:
> 
> ```
> def X: C {
> 	
>     let b=5;
> 			
> }
> ```
I think I've improved it, you tell me :)

I made the base class not a single letter name to distinguish it from the defs. Then I changed the variable name to `var` so that it's distinct from the def names. Finally I rearranged it a bit so that the value of `var` counts up and things get more complex each time.

I'm not sure what I would rename X/Y/Z to. I thought maybe "Inner", "InnerMost" but it wouldn't really be accurate imo. Maybe you have an idea there.


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_tutorial_part_1.ipynb:157
+   "execution_count": 4,
+   "id": "964e9ba5",
+   "metadata": {},
----------------
Eymay wrote:
> This sentence can be moved after the block as it is or can be merged with the previous sentence as a broad introduction to the code block's use case. The issue I had was transitioning from a code block's explanation to the other. It would be good to keep it a single paragraph to introduce each code block like it seems in the [[ https://llvm.org/docs/TableGen/ProgRef.html | Programmer's Reference ]].
Agreed. Moved it down to below the code.

In general I think I flipped back and forth on describing the blocks first, or after the code. I did what felt right but it may be better to pick one style and that probably is description after.


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_tutorial_part_1.ipynb:235
+  },
+  {
+   "cell_type": "code",
----------------
Eymay wrote:
> Is this let statement relevant, it can be confusing if not.
Good spot, this is unused. I think I left it in from an earlier draft.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137085/new/

https://reviews.llvm.org/D137085