[PATCH] D148915: [TableGen] Introduce function and lambda

[David Spickett via Phabricator via llvm-commits]

DavidSpickett added a comment.

Just some doc review, will think about the rest if I get time.



================
Comment at: llvm/docs/TableGen/ProgRef.rst:712
+arguments must precede any optional arguments. The function argument default
+values are evaluated from left to right.
+
----------------
This would benefit from being split, perhaps into bullet points.

* required arguments
* optional arguments
* required must precede optional
* default values are evaluated left to right

Then it's clearer that this is a set of rules that one can apply in order (roughly).


================
Comment at: llvm/docs/TableGen/ProgRef.rst:714
+
+The name of function is treated as a function reference to itself. As a result,
+the function name literals can be used in arguments as values with compatible
----------------
It would be useful to provide an example function just above this. Then after this statement, an example of what "can be used in arguments as values" looks like.


================
Comment at: llvm/docs/TableGen/ProgRef.rst:718
+
+Recursive function is supported(see `Appendix D: Sample Function/Lambda`_).
+
----------------
Usually this would be written as "Recursive functions are" (though you could recurse with only one function, so it's not wrong).


================
Comment at: llvm/docs/TableGen/ProgRef.rst:720
+
+Lambda(anonymous function)
+``````````````````````````
----------------
Space between Lambda and the brackets.


================
Comment at: llvm/docs/TableGen/ProgRef.rst:728
+
+A lambda is a function with anonymous name and all captured variables. It can
+be treated as a function reference to the anonymous function, so it can even
----------------
This could be clearer because it reads like the captured variables are somehow anonymous.

How about: "is an anonymous function that contains all captured variables".


================
Comment at: llvm/docs/TableGen/ProgRef.rst:735
+outer class, multiclass or function; variables defined by ``defvar`` in outer
+scope). In particular, the priorities of variable capture are:
+
----------------
What is the implication of this priority list? If for example I have a `foo` in the inner scope, it will capture that over the `foo` in the outer scope?

An example would help. Something like this after the list. "By applying those priorities we can see that in the example below, `a` is taken from....." 


================
Comment at: llvm/docs/TableGen/ProgRef.rst:739
+
+2. Arguments of lambda.
+
----------------
"of the lambda"


================
Comment at: llvm/docs/TableGen/ProgRef.rst:747
+
+6. Variables in global scope..
+
----------------
Extra `.` here, remove that.


================
Comment at: llvm/docs/TableGen/ProgRef.rst:749
+
+Currently, the fields of a class can't be captued.
+
----------------
captued -> captured


================
Comment at: llvm/docs/TableGen/ProgRef.rst:751
+
+Funciton call
+`````````````
----------------
Capitalise the C on call


================
Comment at: llvm/docs/TableGen/ProgRef.rst:767
+its arguments are wrong). The key point here is that we don't know whether it's
+a new DAG expression or a function call when emitting ``(``.
+
----------------
This "when emitting" doesn't make sense to me, but as stated, I know nothing about parsers :)

What does "emitting" mean here?


================
Comment at: llvm/docs/TableGen/ProgRef.rst:1614
+.. note::
+  Although we can achieve the same goal, but we encourage the user to use
+  function instead. For example above, you can rewrite it to function:
----------------
You don't need the "but" here.


================
Comment at: llvm/docs/TableGen/ProgRef.rst:1615
+  Although we can achieve the same goal, but we encourage the user to use
+  function instead. For example above, you can rewrite it to function:
+
----------------
"using a function", though "to a function" isn't wrong just not usual.


================
Comment at: llvm/docs/TableGen/ProgRef.rst:2178
+
+Appendix D: Sample Function/Lambda
+=========================
----------------
...or instead of the example being above, link to this from the docs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148915