This is an archive of the discontinued LLVM Phabricator instance.

Add new 'let' command to bind arbitrary values into constants.
ClosedPublic

Authored by sbenza on Apr 15 2014, 12:14 PM.

Details

Diff Detail

Event Timeline

pcc added inline comments.Apr 16 2014, 6:44 PM
clang-query/Query.cpp
131

Is this path reachable? If not, we should either remove it or come up with a command syntax for erasing named values.

clang-query/QueryParser.cpp
144

Can you keep these in alphabetical order, please?

194

Ditto.

clang-query/tool/ClangQuery.cpp
122–124

You could pass in a lambda here instead.

sbenza updated this revision to Unknown Object (????).Apr 17 2014, 7:47 AM

Added unlet command. Minor fixes.

clang-query/Query.cpp
131

It was not reachable.
Added the 'unlet' command and updated all the tests for it.

clang-query/QueryParser.cpp
144

Done.
I assumed it wasn't ordered because NoOp is before Help.

194

Done.

clang-query/tool/ClangQuery.cpp
122–124

My bad. I assumed it needed a function pointer.
Fixed.

pcc added inline comments.Apr 17 2014, 12:01 PM
clang-query/Query.h
35

Sorry, missed this one -- alphabetical order, please.

clang-query/QueryParser.cpp
71

You could factor this code out into a completeWord function.

264

It would be nice to support completion for value names here.

270

Can you please use 'endQuery' here so that extra input causes an error?

sbenza updated this revision to Unknown Object (????).Apr 17 2014, 2:30 PM

Minor fixes

clang-query/Query.h
35

Done.

clang-query/QueryParser.cpp
71

How would it help with this change?

264

Yes.
When I added the named values in dynamic::Parser I added a TODO for completion.
The Sema should provide completion for named values, which will allow for completion here and when building matchers.
My next change will add that to dynamic::Parser.

270

Done.

pcc added inline comments.Apr 17 2014, 3:09 PM
clang-query/Query.cpp
131

Sorry, one thing I forgot to mention before: is this the correct behavior if the value does not exist in the map? Should we be printing an error message in that case?

clang-query/QueryParser.cpp
71

I meant that it could help with implementing completion, but if you want to do that separately, that's fine.

264

OK, sounds good.

unittests/clang-query/QueryParserTest.cpp
123

Please also test that extra stuff after the unlet query causes an error.

sbenza added inline comments.Apr 18 2014, 6:35 AM
clang-query/Query.cpp
131

I don't know.
I would say it is not an error because the end state is the same as it would have been if it existed in the map.

unittests/clang-query/QueryParserTest.cpp
123

I'm sorry, but I don't know what extra stuff you are referring to.

pcc added inline comments.Apr 18 2014, 4:19 PM
clang-query/Query.cpp
131

That seems reasonable.

unittests/clang-query/QueryParserTest.cpp
123

Sorry if I wasn't clear. I meant that if for example the query was something like "unlet x foo" we should reject it because of the extra input "foo" at the end.

sbenza updated this revision to Unknown Object (????).Apr 21 2014, 9:18 AM

Added another test for unlet

unittests/clang-query/QueryParserTest.cpp
123

Done.

pcc added inline comments.Apr 22 2014, 11:12 AM
unittests/clang-query/QueryEngineTest.cpp
139

Can you please move this test to QueryParserTest?

sbenza updated this revision to Diff 8739.Apr 22 2014, 1:00 PM
sbenza edited edge metadata.

Moved test case to QueryParserTest

pcc accepted this revision.Apr 22 2014, 1:14 PM
pcc edited edge metadata.

LGTM

This revision is now accepted and ready to land.Apr 22 2014, 1:14 PM
sbenza closed this revision.Apr 23 2014, 7:13 AM

Submitted as r206984