This is an archive of the discontinued LLVM Phabricator instance.

Introduce Registry::getCompletions.
ClosedPublic

Authored by pcc on Nov 18 2013, 12:44 AM.

Details

Summary

This returns a list of valid (and useful) completions for a context (a list
of outer matchers), ordered by decreasing relevance then alphabetically. It
will be used by the matcher parser to implement completion.

Diff Detail

Event Timeline

sbenza added inline comments.Nov 18 2013, 7:52 AM
include/clang/AST/ASTTypeTraits.h
67 ↗(On Diff #5622)

Comment what is the meaning of A<B. It can't be used as a meaningful comparison between the types.

include/clang/ASTMatchers/Dynamic/Registry.h
38 ↗(On Diff #5622)

StringRef

lib/ASTMatchers/Dynamic/Marshallers.h
207 ↗(On Diff #5622)

100?

258 ↗(On Diff #5622)

Comment these.

363 ↗(On Diff #5622)

This is repeated with FixedArgCountMatcherDesc. Factor this out into a function.

382 ↗(On Diff #5622)

You could make this field also const if BuildReturnTypeVector<> returned the vector instead of taking by ref.

541 ↗(On Diff #5622)

Please add a check that all the overloads have the same properties.

549 ↗(On Diff #5622)

This has the potential to give invalid autocomplete.
For example, the first argument only matches Overloads[0], but the second argument autocompletes with Overloads[1] ArgKind.
At least add a note about it. We might want to fix it somehow later on.

605 ↗(On Diff #5622)

You are not using LLVM_OVERRIDE consistently.
You are missing it in many of the overrides.

663 ↗(On Diff #5622)

Is the overload above used if you add this one?

pcc updated this revision to Unknown Object (????).Nov 19 2013, 2:12 AM
  • Address reviewer comments
include/clang/AST/ASTTypeTraits.h
67 ↗(On Diff #5622)

Done.

include/clang/ASTMatchers/Dynamic/Registry.h
38 ↗(On Diff #5622)

Done.

lib/ASTMatchers/Dynamic/Marshallers.h
207 ↗(On Diff #5622)

Arbitrary. I think that if we have an inheritance hierarchy 100 levels deep we probably have bigger problems than this constant :)

258 ↗(On Diff #5622)

I think the function names are self explanatory.

363 ↗(On Diff #5622)

Done.

382 ↗(On Diff #5622)

Probably, but I don't think it matters too much whether this is const or not. All the member functions are const qualified anyway.

541 ↗(On Diff #5622)

Done.

549 ↗(On Diff #5622)

Done. (This also applies to other matcher descriptors so I added the note to the base class.) This will probably have to be done by passing more context information through to getArgKinds.

605 ↗(On Diff #5622)

I'm using it consistently wherever I intend to override a non-pure virtual function.

663 ↗(On Diff #5622)

No. We check on lines 409 and 410 of RegistryTest.cpp that the correct overload is chosen for dyncast matchers.

sbenza added inline comments.Nov 19 2013, 6:41 AM
lib/ASTMatchers/Dynamic/Marshallers.h
605 ↗(On Diff #5622)

I see. Shouldn't it be used for overriding any virtual method, even if it is pure?

663 ↗(On Diff #5622)

What I mean is that if the overload above is not really used, we should get rid of it, and merge VariadicFuncMatcherDesc with DynCastAllOfMatcherDesc.

pcc added inline comments.Nov 22 2013, 5:58 PM
lib/ASTMatchers/Dynamic/Marshallers.h
605 ↗(On Diff #5622)

Well, it's most useful when overriding non-pure virtual member functions because it helps detect otherwise-conforming mistakes.

663 ↗(On Diff #5622)

It's currently used by TypeTraversePolymorphicMatchers and VariadicAllOfMatchers.

pcc updated this revision to Unknown Object (????).Nov 24 2013, 11:52 PM

Rebase.

sbenza accepted this revision.Nov 25 2013, 7:32 AM

LG for me.
@klimek, do you have comments?

pcc closed this revision.Jan 23 2014, 2:54 PM

Closed by commit rL199950 (authored by @pcc).