Page MenuHomePhabricator

[SyntaxTree][Synthesis] Add support for simple Leafs and test based on tree dump
ClosedPublic

Authored by eduucaldas on Sep 11 2020, 12:44 AM.

Diff Detail

Event Timeline

eduucaldas created this revision.Sep 11 2020, 12:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2020, 12:44 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
eduucaldas requested review of this revision.Sep 11 2020, 12:44 AM

Remove unused include

gribozavr2 added inline comments.Sep 11 2020, 1:58 AM
clang/include/clang/Tooling/Syntax/BuildTree.h
30–31
30–31

I think putting token kind first might be more readable at the call site -- going top-down, first the kind (general), then spelling (specific).

clang/lib/Tooling/Syntax/Synthesis.cpp
26

+assert(spelling != nullptr); ?

62

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);
clang/unittests/Tooling/Syntax/SynthesisTest.cpp
66–73

Could you extract the part that I highlighted into a function, and deduplicate it across all tests?

eduucaldas marked 5 inline comments as done.

Answer inline comments

eduucaldas added inline comments.Sep 11 2020, 5:34 AM
clang/lib/Tooling/Syntax/Synthesis.cpp
62

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 Keywords or Punctuators.
As an alternative we could unify the createPunctuatorand 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!

gribozavr2 added inline comments.Sep 11 2020, 5:51 AM
clang/lib/Tooling/Syntax/Synthesis.cpp
62

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

eduucaldas marked an inline comment as done.

createPunctuation, createKeyword -> createLeaf

eduucaldas added inline comments.Sep 11 2020, 9:01 AM
clang/lib/Tooling/Syntax/Synthesis.cpp
62

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.

fix clang-tidy warning

gribozavr2 accepted this revision.Sep 11 2020, 10:46 AM
gribozavr2 added inline comments.
clang/lib/Tooling/Syntax/Synthesis.cpp
49

createLeafLowLevel now seems to be used only once, inline it?

clang/unittests/Tooling/Syntax/SynthesisTest.cpp
46

What does 'C' stand for?

This revision is now accepted and ready to land.Sep 11 2020, 10:46 AM
eduucaldas marked 2 inline comments as done.

Remove createLeafLowLevel

gribozavr2 accepted this revision.Sep 11 2020, 11:26 AM
This revision was landed with ongoing or failed builds.Sep 11 2020, 11:27 AM
This revision was automatically updated to reflect the committed changes.