Page MenuHomePhabricator

NFC: Port QueryParser to StringRef
ClosedPublic

Authored by steveire on Mon, Jan 7, 2:58 PM.

Details

Summary

There is no reason for it to not be a StringRef. Making it one
simplifies existing code, and makes follow-up features easier.

Diff Detail

Event Timeline

steveire created this revision.Mon, Jan 7, 2:58 PM
kristina added a reviewer: kristina.EditedMon, Jan 7, 6:05 PM
kristina added a subscriber: kristina.

LGTM aside from one comment. There's a few style nits however, and I think the lambda on one line could use a few newlines for readability.

clang-query/QueryParser.cpp
39

I don't think this is the best way of handling it, in fact I'm pretty certain you're leaking memory here.

steveire updated this revision to Diff 180626.Tue, Jan 8, 1:51 AM

Run clang-format

steveire marked an inline comment as done.Tue, Jan 8, 1:52 AM

Thanks, I ran clang-format, and I'd rather let it be the rulemaker, rather than try to add newlines in the lambdas :).

clang-query/QueryParser.cpp
39

Hmm, I didn't know we could get a mem leak with StringRef. Can you explain?

aaron.ballman added inline comments.Tue, Jan 8, 8:04 AM
clang-query/QueryParser.cpp
33

Why not just return Line?

39

There's no memory leak here that I can see -- StringRef is non-owning.

40

Why not just return Line here as well?

43–45

Can you use Line.take_until(isWhitespace);?

steveire marked 2 inline comments as done.Tue, Jan 8, 8:39 AM
steveire added inline comments.
clang-query/QueryParser.cpp
33

It is the pre-existing behavior to return a zero-length StringRef with a valid Begin pointer in this case. I think the reason is something to do with code completion. I can check later.

40

That's pre-existing. Seems more clear to leave it as is.

aaron.ballman added inline comments.Tue, Jan 8, 8:46 AM
clang-query/QueryParser.cpp
33

I believe returning Line is identical -- you should wind up with the same "begin" pointer for the returned string and a length of zero (because Line must be empty by this point).

40

I don't have a strong preference, but I raised the point because it seems odd to explicitly clear a StringRef and then return a new, different, empty StringRef on the next line.

steveire marked an inline comment as done.Tue, Jan 8, 8:47 AM
steveire added inline comments.
clang-query/QueryParser.cpp
33

Actually, I can see it by reading the code - P->CompletionPos <= Word.data() in the LexOrCompleteWord ctor woudln't work anymore without this hack. (Working on removing the hack is out of scope of this change).

steveire marked an inline comment as done.Tue, Jan 8, 8:49 AM
steveire added inline comments.
clang-query/QueryParser.cpp
33

Ok, I see your new comment now. I guess that will work. I don't know the behavior of ltrim and whether it preserves that property.

steveire marked an inline comment as done.Tue, Jan 8, 8:51 AM
steveire added inline comments.
clang-query/QueryParser.cpp
40

Well, code changes, and Line may not always be empty here. As it is, this communicates that we're returning an empty StringRef and always will (until this line has a reason to change).

aaron.ballman added inline comments.Tue, Jan 8, 8:56 AM
clang-query/QueryParser.cpp
33

StringRefs are immutable and non-owning, so ltrim() returns Line.begin + someOffset in essence. That should preserve the behavior you need here.

40

Okay, that's good enough for me, thanks!

aaron.ballman accepted this revision.Tue, Jan 8, 9:08 AM

LGTM aside from the nits pointed out; you can fix and commit without another round of review.

This revision is now accepted and ready to land.Tue, Jan 8, 9:08 AM
This revision was automatically updated to reflect the committed changes.