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

Mehmet Eymen Ünay via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 10 10:34:07 PST 2023


Eymay added inline comments.


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_tutorial_part_1.ipynb:15

----------------
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.


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_tutorial_part_1.ipynb:77

----------------
Embedding links could be better


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_tutorial_part_1.ipynb:154

----------------
first let can be enclosed as `let`


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_tutorial_part_1.ipynb:154

----------------
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;
			
}
```


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_tutorial_part_1.ipynb:157

----------------
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 ]].


================
Comment at: llvm/utils/TableGen/jupyter/tablegen_tutorial_part_1.ipynb:235

----------------
Is this let statement relevant, it can be confusing if not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137085



More information about the llvm-commits mailing list