[PATCH] D87495: [SyntaxTree][Synthesis] Add support for simple Leafs and test based on tree dump

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas added inline comments.


================
Comment at: clang/lib/Tooling/Syntax/Synthesis.cpp:51
+  return createLeaf(A, tok::getKeywordSpelling(K), K);
+}
+
----------------
gribozavr2 wrote:
> eduucaldas wrote:
> > gribozavr2 wrote:
> > > Could we make a combined function that does not require the user to make a distinction between punctuation and keywords?
> > > 
> > > We should also allow creating tokens that have a user-specified spelling, for example, identifiers and string literals.
> > > 
> > > So maybe define two functions:
> > > 
> > > ```
> > > // Uses the provided spelling.
> > > syntax::Leaf *createLeaf(syntax::Arena &A, tok::TokenKind K, StringRef Spelling);
> > > 
> > > // Infers spelling if possible.
> > > syntax::Leaf *createLeaf(syntax::Arena &A, tok::TokenKind K);
> > > ```
> > > 
> > First, thanks for the great comments!
> > 
> > > Could we make a combined function that does not require the user to make a distinction between punctuation and keywords?
> > We could! 
> > Bui I wouldn't call it `createLeaf`, as it would work strictly for `Keyword`s or `Punctuator`s. 
> > As an alternative we could unify the `createPunctuator`and `createKeyword` into `createPunctuatorOrKeyword` but I would argue that the previous two names are more readable. Effectively, we would be unifying those 2 functions because they have the same signature.
> > 
> > > We should also allow creating tokens that have a user-specified spelling, for example, identifiers and string literals.
> > That exists already! It is `createLeaf` as is!
> > but I would argue that the previous two names are more readable. Effectively, we would be unifying those 2 functions because they have the same signature.
> 
> I don't think the user cares very much about whether their token is a keyword or a punctuation. Furthermore, most callsites will have the token kind as an argument, so saying that it is a keyword or punctuation is redundant:
> 
> ```
> createKeyword(Arena, tok::kw_if); // Yeah, 'if' is a keyword.
> ```
> 
> The semantics of the combined function I'm proposing is "create a token that has one fixed spelling". The other function is "create a token where the spelling is user-provided".
I think the user cares about whether their token is a keyword.

It's is somewhat redundant yes, but I don't think that hurts, we are still specifying that the keyword being used is `if`.

I see your points, and I don't have a strong opinion so I'm updating the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87495