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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
