[PATCH] Support named values in the autocomplete feature.

Peter Collingbourne peter at pcc.me.uk
Mon May 26 19:28:38 PDT 2014


================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:86
@@ +85,3 @@
+    virtual std::vector<MatcherCompletion>
+    getNamedValueCompletions(llvm::ArrayRef<ArgKind> AcceptedTypes);
+
----------------
Samuel Benzaquen wrote:
> Peter Collingbourne wrote:
> > 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.
> 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.
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.

http://reviews.llvm.org/D3509






More information about the cfe-commits mailing list