This is an archive of the discontinued LLVM Phabricator instance.

[SyntaxTree] Provide iterator-like functions for Lists
AbandonedPublic

Authored by eduucaldas on Sep 22 2020, 9:38 AM.

Details

Reviewers
gribozavr2

Diff Detail

Event Timeline

eduucaldas created this revision.Sep 22 2020, 9:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2020, 9:38 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
eduucaldas requested review of this revision.Sep 22 2020, 9:38 AM
eduucaldas updated this revision to Diff 296994.Oct 8 2020, 9:27 AM
  • [SyntaxTree] Provide iterator for List that iterates through ElementAndDelimiters even for not well-defined lists.
eduucaldas updated this revision to Diff 296996.Oct 8 2020, 9:42 AM

Better comments

eduucaldas updated this revision to Diff 297001.Oct 8 2020, 9:47 AM

Reorganize methods to minimize diffs

Replace auto .. = std::vector(); with std::vector ..;

This is quite low priority, compared to the other branches of work, but it was almost ready work, so I decided to polish it and send it to review now.

clang/include/clang/Tooling/Syntax/Tree.h
302–337

Should we hide some of this? Most of the member functions are a couple of lines, so I decided not to. What is your opinion?

310–317

These are all private static methods, should we hide them from the header?

339–340

I chose to leave it as getBegin and getEnd instead of getElementsAndDelimitersBegin. I did this because the return type of those methods already tells us that we're getting an ElementAndDelimiterIterator.

What do you think?

gribozavr2 added inline comments.Oct 12 2020, 4:46 AM
clang/include/clang/Tooling/Syntax/Tree.h
208

Please also define operator!= even if it is not used yet.

292

"how something looks" or "what something looks like"

302–337

I think it is fine as is.

312

People can use "find usages" to find what uses these methods. Such comments often go out of date.

333

Please also define operator!=.

347

I'd imagine derived classes would have iterators that produce strongly-typed elements, right?

If so, these methods getBegin()/getEnd()/getRange() should have the word "Node" in them.

clang/lib/Tooling/Syntax/Tree.cpp
405

I have a hard time understand what a b x and the comma stand for in these comments.

405–410
407

Ditto?

eduucaldas marked 8 inline comments as done.

Answer comments, TODO: think about templated iterators

eduucaldas marked an inline comment as done.
  • Make ElementAndDelimiterIterator templated.
clang/include/clang/Tooling/Syntax/Tree.h
333

this is defined by the iterato_facade_base, take a look at the comments in this class template

clang/lib/Tooling/Syntax/Tree.cpp
407

Here we could do std::transform(getBegin(), getEnd(), std::back_inserter(result), [](ElementAndDelimiter ED){return ED.element;}). Is that more idiomatic?

Haven't yet implemented BeforeBegin, waiting for a heads up on the patch as is.

eduucaldas added inline comments.Oct 14 2020, 8:38 AM
clang/include/clang/Tooling/Syntax/Tree.h
302–303

Since we're gonna provide strongly-typed iterators, I make the Iterator a class template.

I keep the base functions as they were, getElementAndDelimiterAfter..., but I cast their result using castToElementType.

Another option would be to make the base functions also templated and not perform any casting.

We'll use castToElementType to provide the strongly-typed iterators as well.

  • [SyntaxTree] ElementAndDelimiterIterator is polymorphic and supports BeforeBegin iterator
eduucaldas marked 2 inline comments as not done.

Add tests, ElementAndDelimiter are input iterators

rename getBeforeBegin

eduucaldas abandoned this revision.Oct 26 2020, 9:26 AM
eduucaldas added inline comments.
clang/include/clang/Tooling/Syntax/Tree.h
302–303

Ignore comment above

313

Since NodeRole is forward declared, we cannot access NodeRole::ListElement within the header, this is my work-around.

Another possibility would be to not forward declare and put NodeRole definition here. I think this is a bad choice because most roles are linked to syntax.

I declare isElement as a static private member function of List from lack of better place, I accept suggestions.

451

I have a hard time finding a name that is expressive enough and concise enough. The current one seems to imply that we're getting a node...

Alternatives:

  • getBeforeBeginWithElementAsNode
  • getBeforeBeginWithNodes
  • getBeforeBeginBase
  • getBeforeBeginNodeAndDelimiter