Page MenuHomePhabricator

[SyntaxTree] Provide iterator-like functions for Lists
Needs ReviewPublic

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

Details

Reviewers
gribozavr2

Diff Detail

Event Timeline

eduucaldas created this revision.Tue, Sep 22, 9:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Sep 22, 9:38 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
eduucaldas requested review of this revision.Tue, Sep 22, 9:38 AM
eduucaldas updated this revision to Diff 296994.Thu, Oct 8, 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.Thu, Oct 8, 9:42 AM

Better comments

eduucaldas updated this revision to Diff 297001.Thu, Oct 8, 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
241–276

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?

278–279

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?

441–448

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

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

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

228

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

241–276

I think it is fine as is.

272

Please also define operator!=.

286

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.

443

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

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

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

340–345
349

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
272

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

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

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.Wed, Oct 14, 8:38 AM
clang/include/clang/Tooling/Syntax/Tree.h
241–242

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