[PATCH] D108283: [flang][driver] Add documentation for Plugins

[Peter Klausler via Phabricator via llvm-commits]

klausler added a comment.

In D108283#2994130 <https://reviews.llvm.org/D108283#2994130>, @kiranchandramohan wrote:

> LGTM.
>
> @klausler We are exposing the Parser::Walk framework using a Visitor Class with Pre and Post functions as the mechanism for traversing the parse-tree in flang plugins. I hope this is OK with you.

Well, I don't like the Pre()/Post() paradigm much, myself, and would prefer an API based on function objects (i.e., classes with overloaded operator()(const parser::xxx &) member functions) if I had to write a traversal client.  See Evaluate/traverse.h for how this looks for Expr<>; there's a default traversal template that visits everything, and it's instantiated upon its subclasses ("CRTP") which can override its behavior for targets of interest as it likes.  The Pre()/Post() paradigm tends to lead to confusion when both are present and performing stateful actions, and it's too easy to introduce bugs by changing one and not the other.  A function-object visitation template for parse tree data structures could be built pretty easily using the existing visitor as a reference, and then used as a new substrate for that visitor.  That's more of a project, obviously, and may only be worth doing if you agreed with my opinion on the API (which you're probably regretting asking me for by this point; sorry!).

Second: we just can't commit to the current parse tree classes and enums being a stable API forever, and the documentation should have disclaimers that plug-ins may stop compiling or working as the parse tree data structures evolve over time to accommodate future language standards, bugfixing, and basic clean-up.

Third, be advised: I did not intend this parse tree to be the front-end's output, and it has many of the hallmarks of an "internal" data structure meant only to survive through semantic checking, at which point it was meant to be raised to a more abstract and less messy representation suitable for lowering as well as for exposure to tools and plug-ins as a stable and *mutable* semantic tree.  FIR happened instead.  This parse tree was designed to be really inexpensive to create and discard as the product of a fast backtracking parser, and trade-offs were made in that direction.  Consequently, the current parse tree is really hard to edit in situ (as we do to correct a few syntactically ambiguous parsing cases) and its definition explicitly disables all of its copy constructors.  (The copy constructors are intentionally disabled to ensure that the parser combinators always use fast move construction.)  If you base the plug-in API on the current parse tree, I think it likely that one of your early tool-writing clients will want to use it to duplicate some code for some transformation, and they will be sad.  So manage expectations carefully.  It's called the parse tree, not an AST, for good reasons.

Last: besides editing the parse tree to fix up ambiguous parses with symbolic information, Semantics also decorates the parse tree with unowned pointers to symbol table entries and owned pointers to the strongly typed Expr<> representation of expressions.  Tool writers will probably have to be aware of the Scope and Symbol classes, and also know that they'll have to re-analyze any expressions if they modify their parse trees (unless they want to modify Expr<>'s in situ and perhaps make them inconsistent with the parse trees to which they're attached).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108283