Page MenuHomePhabricator

Support named values in the autocomplete feature.
ClosedPublic

Authored by sbenza on Apr 25 2014, 6:50 AM.

Details

Summary

This includes:

  • Passing a Sema to completeExpression to allow for named values in the expression.
  • Update the Sema interface to include completion for matchers and values.
  • Change the parser to use the Sema for completion, instead of going directly to Registry.

Diff Detail

Repository
rL LLVM

Event Timeline

sbenza updated this revision to Diff 8839.Apr 25 2014, 6:50 AM
sbenza retitled this revision from to Support named values in the autocomplete feature..
sbenza updated this object.
sbenza edited the test plan for this revision. (Show Details)
sbenza added a reviewer: pcc.
sbenza added a subscriber: Unknown Object (MLST).
pcc edited edge metadata.May 16 2014, 7:18 PM

Apologies for not reviewing this earlier, it slipped through the cracks. In future feel free to ping if I haven't looked at a change within a week.

include/clang/ASTMatchers/Dynamic/Parser.h
86 ↗(On Diff #8839)

This approach requires a bunch of common logic to be duplicated between individual Sema implementations. Instead, you could have a virtual function in this class that returns (a const reference to) a map of named values, which would be called by this function and getNamedValue.

You might also consider changing the parser API to pass the map separately (along with Sema) to the parser. That way, you could also avoid complicating Sema too much as a result of having to allow completeExpression to take custom Semas.

lib/ASTMatchers/Dynamic/Parser.cpp
578 ↗(On Diff #8839)

Nit: specificity.

unittests/ASTMatchers/Dynamic/RegistryTest.cpp
456 ↗(On Diff #8839)

Instead of removing these checks, you could probably change them to instead check the relative specificity of the results.

sbenza added inline comments.May 19 2014, 6:12 AM
include/clang/ASTMatchers/Dynamic/Parser.h
86 ↗(On Diff #8839)

Having this in the Sema object is following the same logic as having the matchers.
It allows an implementation of it to do whatever it wants. It could be backed by an in-memory std::map, be generated on demand, or by some other source.
Earlier, I added 'RegistrySema' because it was common to use that one.
I was thinking in providing a MappedValueSema, and make both of these mix-ins that the user can add to their implementations.

wrt duplicated logic, I added most of the required logic in the support classes (VariantValue, etc). The implementation of this method of a backing string->value map is just a few lines.

pcc added inline comments.May 26 2014, 7:28 PM
include/clang/ASTMatchers/Dynamic/Parser.h
86 ↗(On Diff #8839)

I realize why you added this flexibility, and I sort of see the value in it, but I'm not convinced that it is necessary.

If I were designing this, I would make the std::map part of the interface to the parser, and defer the design of any additional flexibility for when it is needed, so that I'd have something concrete to base my design on. For example, it could be done incrementally on top of the std::map design by adding something to Sema that lets a client augment the contents of the map. I won't insist on you changing your current design though.

sbenza updated this revision to Diff 12205.Aug 5 2014, 11:21 AM
sbenza edited edge metadata.

Reworked the changelist to pass a map of values instead of using the Sema for it.

Sorry for the very long delay.
I applied your suggestions.

include/clang/ASTMatchers/Dynamic/Parser.h
86 ↗(On Diff #8839)

Followed your suggestion.
To make it consistent I had to remove the getNamedValue() method also. This will break clang-query temporarily.

lib/ASTMatchers/Dynamic/Parser.cpp
578 ↗(On Diff #8839)

Fixed

sbenza updated this revision to Diff 12248.Aug 6 2014, 1:01 PM

Print the name in the "declaration" to show it on the autocomplete suggestion.

pcc added a comment.Aug 7 2014, 6:17 PM

Thank you, this looks close.

include/clang/ASTMatchers/Dynamic/Parser.h
86 ↗(On Diff #8839)

It looks like it should be easy to fix clang-query. Can you please also upload a diff for clang-tools-extra?

lib/ASTMatchers/Dynamic/Parser.cpp
457 ↗(On Diff #12248)

Shouldn't this require updates to the unit tests? If not, why not?

sbenza updated this revision to Diff 12307.Aug 8 2014, 7:46 AM

Fix the tests to pass with the new behavior of completion logic.

sbenza added inline comments.Aug 8 2014, 7:47 AM
include/clang/ASTMatchers/Dynamic/Parser.h
86 ↗(On Diff #8839)

It is easy to fix. I have another change for that, but I am not sure if I can send them atomically.
Aren't they in different repos?

lib/ASTMatchers/Dynamic/Parser.cpp
457 ↗(On Diff #12248)

Yes. It does. Apparently I ran the wrong tests before.

pcc added inline comments.Aug 8 2014, 11:39 AM
include/clang/ASTMatchers/Dynamic/Parser.h
86 ↗(On Diff #8839)

You can just create another differential revision.

The change in clang-query in diff D4851

pcc accepted this revision.Aug 11 2014, 2:22 PM
pcc edited edge metadata.

LGTM

Please remember to commit D4851 at the same time.

This revision is now accepted and ready to land.Aug 11 2014, 2:22 PM
sbenza closed this revision.Aug 12 2014, 2:20 PM
sbenza updated this revision to Diff 12417.

Closed by commit rL215472 (authored by @sbenza).