This is an archive of the discontinued LLVM Phabricator instance.

Optionally add code completion results for arrow instead of dot
ClosedPublic

Authored by yvvan on Dec 22 2017, 3:00 AM.

Details

Summary

Currently getting such completions requires source correction, reparsing and calling completion again. And if it shows no results and rollback is required then it costs one more reparse.
With this change it's possible to get all results which can be later filtered to split changes which require correction.

Diff Detail

Event Timeline

yvvan created this revision.Dec 22 2017, 3:00 AM
yvvan added a subscriber: nik.
yvvan updated this revision to Diff 128509.Jan 3 2018, 3:01 AM

Update CIndex minor version, add call to libclang.exports

yvvan updated this revision to Diff 130384.Jan 18 2018, 2:58 AM

Rebased. Applies for current master.

Also ping again...

One more Ping!

Sorry for not taking a look for a long time. I think that would be a very useful feature to have.
Here are a few comments (also see the inline comments)

  • Please add tests
  • How would the clients know how to correct dot to arrow? It'd be cool if code completion results to provide a replacement for that (e.g. return a FixItHint that should be applied along with the completion items that have RequiresDotToArrowCorrection == true).
  • We should probably have a more general option, i.e. AllowCorrections. We could reuse it for other advanced completion scenarios.
  • This patch calls HandleCodeCompleteResults twice and it breaks at least one existing client (clangd) and, arguably, makes the contract of CodeCompleteConsumer more complicated. I have a patch that addresses that: D42474. The change also moves some of the code to parser, arguably making the overall patch.
include/clang/Sema/CodeCompleteConsumer.h
34

Maybe remove the default argument?
This constructor seems to only be called CodeCompletionBuilder::TakeString.

34

Maybe remove = false, initialize in constructor to be consistent with how other members are initialized?

34

Maybe remove the default argument too?
This method seems to have only 5 callsites, they should be very easy to update.

include/clang/Sema/CodeCompleteOptions.h
6

Could we make a more general option (say, IncludeCorrections)?
Arrow instead of dot is useful, but we might try to add more items that require various corrections when doing completions and it would be nice to reuse the option between those.

lib/Sema/SemaCodeComplete.cpp
49

The code here seems to replicate what the parser does, so certifying that's it's correct is hard and we may not pick up the changes when something changes in parser.
I suggest we compute the Expr* Base for an alternate operator and pass it to code complete function. Here's a change that does that, feel free to pick it up unless you have objections to my line of thought: D42474 (it's missing marking the results with proper flags, so that's something that should be extended).

I also propose to try correcting in both directions, i.e. here's how completion completion could work with this feature:

struct X { int foobarbaz; };

void test() {
  X var;
  X* ptr;
  std::unique_ptr<X> uptr;
   
  var->foobar // replace with var.foobarbaz
  ptr.foobar // replace with ptr->foobar
  uptr->get // replace with uptr.get
  uptr.foobar // replace with uptr->foobarbaz
}
yvvan marked 4 inline comments as done.Jan 29 2018, 3:57 AM

@ilya-biryukov
Thanks a lot for you comments and for the provided code replacement. I'm checking now how it works and will continue addressing other comments.

yvvan updated this revision to Diff 132146.Jan 31 2018, 5:25 AM

Use D42474 code. Add missing parts and tests for errors and fixits.

Sorry for not getting back on this earlier.

I wanted to discuss whether returning correction as a enum value is a proper interface for users of libclang.
It seems easy to replace . with -> inside clang, properly handling all the tricky cases (tokens coming from macro substitution, etc) and making sure it's a viable option (again, macro substitutions may make it non-viable).
And even though it's not hard to implement things like "replace . with ->" for any particular editor, we still need to do that for every small correction that may we want to add to the completion in every editor.

I propose to return something similar to a FixItHint from Diagnostic.h, i.e. description of a simple text edit that should be applied before inserting the completion identifier. In that case, we may hope that every editor will implement the corrections once, and we could add more features into code completions that will be immediately picked up by all the editors that support it.
WDYT?

yvvan added a comment.Feb 21 2018, 4:57 AM

I've already added hints in this patch and it also do not add extra completions unless the flag IncludeCorrections is set. So this will not force editors to use corrections.

Did you mean that you think it's good to have extra fixit hints even if this flag is not set?

yvvan added a comment.Feb 22 2018, 1:15 AM

Or is your idea is to return the char sequence instead to use this correction in some universal way?

Or is your idea is to return the char sequence instead to use this correction in some universal way?

Exactly. Editors that implement corrections would pick up any new corrections automatically, rather than implementing each specific correction separately.

yvvan updated this revision to Diff 138139.Mar 13 2018, 2:01 AM

Return possibly required corrections in the string form

Sorry for the delay.

include/clang/Sema/CodeCompleteConsumer.h
565

Having a string replacement without an actual range to replace still moves a lot of responsibility onto the clients. We should probably model corrections after the fix-its for diagnostics:

  • replacements need to provide a range to be replaced, alongside with a text for the replacement,
  • we should provide a list of edits to allow corrections that touch two separate pieces of code.

For the fix-its in the diagnostics, see CXLoadedDiagnostic.h for an interface exported via libclang and Diagnostic.h for an interface exported in C++ API.
WDYT?

yvvan added inline comments.Mar 21 2018, 1:40 AM
include/clang/Sema/CodeCompleteConsumer.h
565

I thought that fixits always exist separately from diagnostics. I will check this out, thanks.

yvvan added inline comments.Mar 28 2018, 7:25 AM
include/clang/Sema/CodeCompleteConsumer.h
565

I've looked into diagnostics and realized that i already use them here but the are just not forwarded to completion results which is of source possible.

I have a draft with Optional<FixItHint> instead of Optional<std::string>
It allows to use further both string and replaced/removed range and also mostly uses the same code as already provided in this patch since I already generated FixItHint for Diagnostics (line 4117 lib/Sema/SemaCodeComplete.cpp in this patch)

Is that fine with you?

Sorry for the late response, was on vacation.

include/clang/Sema/CodeCompleteConsumer.h
565

Sounds good.
One caveat is that we should probably go with std::vector<FixItHint> instead of Optional<FixItHint>. This would mirror what clang::Diagnostic does.
"Dot to arrow" will only ever add one element there, but more intricate fixes might require more than a single edit.

yvvan updated this revision to Diff 143566.Apr 23 2018, 8:08 AM

Use wrapped FixItHint to keep corrections.

Can be quite easily changed from Optional to some container if required.

yvvan updated this revision to Diff 143572.Apr 23 2018, 8:40 AM

Use vector instead of optional as Ilya suggested

Important: please upload the patch with full context diff

include/clang-c/Index.h
5278 ↗(On Diff #143572)

This implies that "dot to arrow" is the only available correction. Maybe rephrase to mention that others are possible in theory?
E.g.

Whether to include completion items with corrections (small fix-its), e.g. change '.' to '->' on member access, etc.
include/clang/Sema/CodeCompleteConsumer.h
415

Why do we need this wrapper?
It seems that storing SourceManager and LangOpts in each fix-it is clearly confusing (they are the same for all diags in the file).
All clients that want to access the fix-it should have a reference to an existing SourceManager, right?

577

Storing fix-its in CodeCompletionString seems like to be against its original intention, i.e. now it depends on SourceLocations, which require SourceManager, etc.
Is there a way to get to a CodeCompletionResult from libclang? Storing fix-its there would be ideal.

704

Maybe accept the vector by value instead of const reference to allow the clients to std::move the argument and avoid copies?

yvvan added inline comments.Apr 25 2018, 3:02 AM
include/clang-c/Index.h
5278 ↗(On Diff #143572)

thanks, forgot to change that one

include/clang/Sema/CodeCompleteConsumer.h
415

My first version took source manager and language options from CXCodeCompleteResults (which is in fact AllocatedCXCodeCompleteResults) so I needed to pass it as an extra parameter which looks kind of ugly,

The single CXCodeCompleteResult does not have them. But I can leave CXCodeCompleteResults pointer as a parameter to avoid this wrapper.

And since it's a libclang design part it probably should be fixed/workarounded there.

577

CodeCompletionString is an abstraction used by libclang to store everything. So every completion detail getter in libclang works through it.

CXCompletionResult stores only cursor kind and a pointer to that string.

704

but if it's accepted by value - it's one copy already by default

Instead I can add one more constructor with rvalue reference.

yvvan updated this revision to Diff 144076.Apr 26 2018, 2:19 AM

Wrapper around FIxItHint is removed, other review comments are addressed .

Could you upload the patch with full context?

include/clang/Sema/CodeCompleteConsumer.h
600

NIT: return llvm::ArrayRef<FixItHint> instead.

704

If it's accepted by value, it's copy by default for l-values, but callers have an option to std::move explicitly.
Having an r-value ref overload prevents from calling with l-values (e.g. local variables), i.e. requires either an explicitly copy to a temporary or std::move.

In general, I would still suggest passing by-value, unless it's particularly important to prohibit extra copies (e.g. for performance reasons, but I don't think it's the case here).

1040

Mention other corrections are possible in this comment too?

include/clang/Sema/CodeCompleteOptions.h
43

Mention other corrections are possible in this comment too?

yvvan updated this revision to Diff 144285.Apr 26 2018, 11:51 PM
yvvan marked 4 inline comments as done.

Minor clean-up according to the last comments

include/clang/Sema/CodeCompleteConsumer.h
704

Ok, it was always a bit unclear what compiler does in such cases but I've found other references that it's actually a move in that case.

So it's changed to pass-by-value in the next diff update.

1040

Thanks!

Sorry for the delay. I really like the direction of this patch!
What's left is defining the semantics of corrections more thoroughly to make sure we don't have tricky corner cases that users of the API can't deal with.

PS. This patch is still lacking full context of the diff.
Please use arc or send diff with full context.

include/clang-c/Index.h
5220 ↗(On Diff #144285)

I'm a bit vary about exposing source manager and language options in the API just for the sake of corrections.
I suggest we add an extra parameter of an existing class (CXCodeCompleteResults) instead and remove the corresponding helpers to get source manager and language options:
clang_getCompletionNumCorrections(CXCompletionString completion_string, CXCodeCompleteResults* results);

include/clang/Sema/CodeCompleteConsumer.h
564

Adding some docs here would be useful. These fix-its could be interpreted too broadly at this point.
I suggest the following semantics and the comment:

/// \brief FixIts that *must* be applied before inserting the text for the corresponding completion item.
/// Completion items with non-empty fixits will not be returned by default, they should be explicitly requested by setting CompletionOptions::IncludeCorrections.
/// For the editors to be able to compute position of the cursor for the completion item itself, the following conditions are guaranteed to hold for RemoveRange of the stored fixits:
///  - Ranges in the fixits are guaranteed to never contain the completion point (or identifier under completion point, if any) inside them, except at the start or at the end of the range.
///  - If a fixit range starts or ends with completion point (or starts or ends after the identifier under completion point), it will contain at least one character. It allows to unambiguously recompute completion point after applying the fixit.
/// The intuition is that provided fixits change code around the identifier we complete, but are not allowed to touch the identifier itself or the completion point.
/// One example of completion items with corrections are the ones replacing '.' with '->' and vice versa:
///      std::unique_ptr<std::vector<int>> vec_ptr;
///      vec_ptr.^  // completion returns an item 'push_back', replacing '.' with '->'
///      vec_ptr->^ // completion returns an item 'release', replacing '->' with '.'let's put

Do those invariants sound reasonable? Could we add asserts that they hold when constructing the completion results?

565

I wonder if we should call them Fixits instead?
To follow the pattern for diagnostics.

tools/c-index-test/c-index-test.c
2346 ↗(On Diff #144285)

We should print all provided fixits in the test code

2350 ↗(On Diff #144285)

maybe add a source range or source of the replaced text to the printf here?
e.g. "replaces '.' with '->'"?

tools/libclang/CIndexCodeCompletion.cpp
340 ↗(On Diff #144285)

Maybe return a struct containing both the string and CXSourceRange instead of this output parameter?
It does not make sense to get a string without the corresponding source range.
How does libclang return fixits for diagnostics? We probably want to follow the same pattern here.

yvvan added inline comments.May 8 2018, 4:02 AM
include/clang-c/Index.h
5220 ↗(On Diff #144285)

Haha, I had that interface initially but thought it's too ugly and needs to be improved :)

include/clang/Sema/CodeCompleteConsumer.h
564

Thanks! I usually feel the lack of patience when writing descriptions :)

564

"Could we add asserts that they hold when constructing the completion results"

The current way of creating them actually assures that by default. And to check it once again it's required to change many things.
Starting with the fact that there is no SourceRange contains() methos or something similar,

565

Ok, let's call them fixit-s

yvvan updated this revision to Diff 145664.May 8 2018, 4:03 AM

Address comments and provide diff with full context

ilya-biryukov added inline comments.May 8 2018, 8:18 AM
include/clang-c/Index.h
5237 ↗(On Diff #145664)

This seems too large for a brief comment :-)
Maybe break with a newline after the first sentence.

include/clang/Sema/CodeCompleteConsumer.h
564

The current way of creating them actually assures that by default. And to check it once again it's required to change many things.

It's still nice to have a sanity check there, mostly for the sake of new checks that are gonna get added.
But even the current one is tricky, since it may actually contain a range that ends exactly at the cursor (when completing right after the dot/arrow foo->|).

Starting with the fact that there is no SourceRange contains() methos or something similar,

Moreover, SourceRange doesn't have clear semantics AFAIK. It can either be a half-open or closed range.
Let's add a FIXME, at least, that we should add those asserts later.

564

I suggest moving this doc comment to getFixits method, as it is the public interface.

569

We can't store fixits in CodeCompletionString, it should not contain source locatins, which are only valid for the lifetime of SourceManager.
Note that CCS has a lifetime independent of both the SourceManager and the AST.
Storing them in CodeCompletionResult should be good enough for all our use-cases.

785

We shouldn't duplicate such a large comment in too many places, it would be impossible to keep it in sync.
I would suggest only keeping it in CodeCompletionResult and add a reference to it in other places.
libclang is a bit tricky, though. It is distributed without other LLVM headers, right?

yvvan marked 3 inline comments as done.May 9 2018, 7:12 AM

I have some failing tests... So I will update the diff a bit later (Friday most likely)

include/clang/Sema/CodeCompleteConsumer.h
564

It's removed from CodeCompletionString therefore it's now only above the public declaration.

569

To support that I had to make libclang calls even worse but whatever.

785

I will keep the comment only here and in libclang

yvvan updated this revision to Diff 146289.May 11 2018, 2:23 AM
yvvan marked 3 inline comments as done.

Only keep small completion fixit-s in CodeCompletionResults.
Change libclang calls to overcome that and not use CXCompletionString

yvvan added a comment.EditedMay 14 2018, 3:37 AM

ping

I hope this review will be over some day :)

I hope this review will be over some day :)

Sorry for keeping it that long.
FWIW, I think we keep jumping back and forth between the clang and libclang changes here.
I should've suggested splitting the change into two earlier.

I promise to promptly review the changes, let's try getting it in this week. I am very excited to have this feature!

lib/Sema/SemaCodeComplete.cpp
4036

This FIXME should probably go into the CompletionResult constructor, we want to check the invariants in a single place.

4047

Why do we need to report an extra diagnostic here in completion?
Our completion items contain all the relevant information for actually doing the change, and having different diagnostics in completion vs non-completion modes does not seem like the right way to do things.

tools/libclang/CIndexCodeCompletion.cpp
309 ↗(On Diff #146289)

Storing vector<vector<>> here makes looks like a hack. Even though it might seem more tricky, I suggest storing an opaque pointer to vector<FixItHint> in each CXCompletionResult. Managing the lifetime of vectors in the AllocatedCXCodeCompleteResults seems totally fine, but there should be a way to get to the fixits in a similar way we can get to the completion string.
More concretely, I suggest the following API:

// === Index.h
typedef void* CXCompletionFixIts;
typedef struct {
   // ...
   CXCompletionFixIts FixIts;
};

// Get the number of fix-its.
unsigned clang_getCompletionNumFixIts(CXCompletionResult *Result);
// ... Similar to getDiagnosticFixIt
CXString clang_getCompletionFixIt((CXCompletionResult *Result, unsigned fixit_index, CXSourceRange *replacement_range);



// === Impl.cpp
struct AllocatedCXCodeCompleteResults : public CXCodeCompleteResults {
// .....
// Pool for allocating non-empty fixit vectors in the CXCompletionResult.
std::vector<std::vector<FixItHint>> FixItsVector
};

unsigned clang_getCompletionNumFixIts(CXCompletionResult *Result) {
  auto* FixIts = static_cast<std::vector<FixItHint>*>(Result->FixIts);
  if (!FixIts)
    return 0;
  return FixIts->size();
}
yvvan added a comment.May 14 2018, 5:49 AM

I should've suggested splitting the change into two earlier.

Next time I will do that from the beginning :)

tools/libclang/CIndexCodeCompletion.cpp
309 ↗(On Diff #146289)

unsigned clang_getCompletionNumFixIts(CXCompletionResult *Result);

Do you mean appending CXCompletionResult struct with the "CXCompletionFixIts FixIts" member? Doesn't it break binary compatibility (changing the struct size)?

ilya-biryukov added inline comments.May 14 2018, 6:46 AM
tools/libclang/CIndexCodeCompletion.cpp
309 ↗(On Diff #146289)

Ah, you're right. If libclang promises binary compatibility with binaries compiled from headers with previous versions, we're not allowed to do this change.
I'm not sure what's the best way to do that, probably the interface you propose does what we want with the least amount of friction.
I'd be fine with leaving as is, but given that libclang has a stable interface, I'd get more opinions from someone who owns the code before adding this. (Another reason to split this change into two?)

A possible alternative that won't break binary compatibility would be to change the opaque CXCompletionString to point into a struct that has both CodeCompletionString and the fixits vector, but that means finding all use-sites of CXCompletionString and that might be tricky.

yvvan updated this revision to Diff 146749.May 14 2018, 11:19 PM
yvvan marked 2 inline comments as done.

Only C++ part, libclang part has moved to https://reviews.llvm.org/D46862

yvvan added inline comments.May 14 2018, 11:22 PM
lib/Sema/SemaCodeComplete.cpp
4047

In current version of this patch it's not needed anymore. Removed.

tools/libclang/CIndexCodeCompletion.cpp
309 ↗(On Diff #146289)

I already tried to have it in CXCompletionString. And I've even found and replaced all usages. But there's a bigger issue: CXCompletionString does not have dispose method and it exists not only in context of CXCompletionChunkKind (for example as a return value of clang_getCompletionChunkCompletionString). It's easy to add such dispose method but it will introduce leaks in already existing code bases.

By all that I mean that "the interface you propose does what we want with the least amount of friction".

Maybe move the tests back to this revision?
There are tests for code completion that don't depend on libindex or libclang and use clang -cc1 directly, i.e. tools/clang/test/CodeComplete. This would require adding a flag to clang and patching up PrintingCodeCompleConsumer to print the fix-its, but that should be easy. Moreover, it's useful to have the option to show those completions in clang anyway.

include/clang/Basic/DiagnosticSemaKinds.td
5715 ↗(On Diff #146749)

This is not used anymore, right? Maybe remove it?

include/clang/Sema/CodeCompleteConsumer.h
807

NIT: reflow on this comment broke formatting, maybe keep it unindented instead? I.e., this should give better formatting:

///      std::unique_ptr<std::vector<int>> vec_ptr;
///      vec_ptr.^ 
///      vec_ptr->^
///
///  In 'vec_ptr.^', one of completion items is 'push_back', it requires replacing '.' with '->'.
///  In 'vec_ptr->^', one of completion items is 'release', it requires replacing '.' with '->'.
lib/Sema/SemaCodeComplete.cpp
3954

Maybe pass a single fix-it here too with a more descriptive name?
It would make the code more readable.

4025

Maybe pass a single fix-it with more descriptive name, e.g. Optional<FixItHint> AccessOpFixIt?

4111

This FIXME should probably live in CodeCompletionResult constructor or ResultBuilder::AddResult. It's pretty obvious that this specific code does the right thing, but it may be non-obvious about more generic code that stores a vector<FixItHint>

test/SemaCXX/member-expr.cpp
193 ↗(On Diff #146749)

Is this still needed after we removed the diagnostics code?

tools/libclang/CIndexCodeCompletion.cpp
309 ↗(On Diff #146289)

I already tried to have it in CXCompletionString. And I've even found and replaced all usages. But there's a bigger issue: CXCompletionString does not have dispose method and it exists not only in context of CXCompletionChunkKind (for example as a return value of clang_getCompletionChunkCompletionString). It's easy to add such dispose method but it will introduce leaks in already existing code bases.

Yeah, that's probably too compilcated. And it does not make sense for completion strings inside chunks to have fix-its, the fix-its only make sense for the completion item as a whole.

yvvan added a comment.May 15 2018, 3:29 AM

I will add more tests...

test/SemaCXX/member-expr.cpp
193 ↗(On Diff #146749)

Yes, even if it does not fail it is not related to that change anymore

yvvan updated this revision to Diff 147039.May 16 2018, 3:51 AM
yvvan marked 4 inline comments as done.

Append PrintingCodeCompleteConsumer and CompilerInvocation options, add CodeCompletion tests.

Sorry, a few more things and we're good to go.

include/clang/Driver/CC1Options.td
449

NIT: maybe simplify to "which require small fix-its"?

lib/Frontend/CompilerInvocation.cpp
1541

Following naming convention of other options, maybe use OPT_code_completion_with_fixits? (i.e. none of them start with include)

lib/Sema/CodeCompleteConsumer.cpp
559

Unfortunately, that might not work for some ranges, see docs of CharSourceRange:

/// In the token range case, the
/// size of the last token must be measured to determine the actual end of the
/// range.

The TextDiagnostic::emitParseableFixits handles it, I suggest we do it similarly:

FixItHint*I = //....;
SourceLocation BLoc = I->RemoveRange.getBegin();
SourceLocation ELoc = I->RemoveRange.getEnd();

std::pair<FileID, unsigned> BInfo = SM.getDecomposedLoc(BLoc);
std::pair<FileID, unsigned> EInfo = SM.getDecomposedLoc(ELoc);

// Adjust for token ranges.
if (I->RemoveRange.isTokenRange())
  EInfo.second += Lexer::MeasureTokenLength(ELoc, SM, LangOpts);

// We specifically do not do word-wrapping or tab-expansion here,
// because this is supposed to be easy to parse.
PresumedLoc PLoc = SM.getPresumedLoc(BLoc);
if (PLoc.isInvalid())
  break;

OS << "fix-it:\"";
OS.write_escaped(PLoc.getFilename());
OS << "\":{" << SM.getLineNumber(BInfo.first, BInfo.second)
  << ':' << SM.getColumnNumber(BInfo.first, BInfo.second)
  << '-' << SM.getLineNumber(EInfo.first, EInfo.second)
  << ':' << SM.getColumnNumber(EInfo.first, EInfo.second)
  << "}:\"";
OS.write_escaped(I->CodeToInsert);
OS << "\"\n";
lib/Sema/SemaCodeComplete.cpp
4109

I'd use token ranges here to avoid assumptions about sizes of tokens, e.g. CreateReplacemen(CharSourceRange::getTokenRange(OpLoc, OpLoc), IsArrow ? '.' : '->')
There are complicated cases like \ that end of the line and macros and it's definitely better to use an abstraction that hides those cases.

yvvan added inline comments.May 22 2018, 2:42 AM
lib/Sema/CodeCompleteConsumer.cpp
559

yeah. sorry. it's always like that with source ranges. thanks for a hint

lib/Sema/SemaCodeComplete.cpp
4109

the problem is that I need the range, not just a location.... and I don't know how to extract it here. is there a way to get next token location here?

ilya-biryukov added inline comments.May 22 2018, 2:58 AM
lib/Sema/SemaCodeComplete.cpp
4109

IIUC, using CharSourceRange::getTokenRange(OpLoc, OpLoc) will do what you want.
When CharSourceRange is a token range, the client code should measurements the token length and the producing code would use the starting location of the last token.
Here are the relevant bits from the comment of the CharSourceRange:

/// In the token range case, the
/// size of the last token must be measured to determine the actual end of the
/// range.
yvvan added inline comments.May 22 2018, 3:07 AM
lib/Sema/SemaCodeComplete.cpp
4109

ok, i will recheck that it does calculate the token range

yvvan updated this revision to Diff 148164.May 23 2018, 12:06 AM
yvvan marked 4 inline comments as done.

I have only a few nits left, otherwise looks good. Thanks for making this change! Really excited for it to get in finally!

include/clang/Driver/CC1Options.td
449

NIT: there's a redundant space before the closing .

include/clang/Sema/CodeCompleteConsumer.h
785

NIT: remove \brief from the start of the comment. Those recently got removed from all of llvm

1058

NIT: another redundant \brief

include/clang/Sema/CodeCompleteOptions.h
42

NIT: Maybe s/Show also results/Include results?

lib/Sema/SemaCodeComplete.cpp
4115

NIT: maybe swap the two cases to do the non-fixit ones first? It is the most common case, so it should probably go first.

yvvan marked 4 inline comments as done.May 24 2018, 11:31 PM
yvvan added inline comments.
lib/Sema/SemaCodeComplete.cpp
4115

The actual completion should be the last because of the "Completion context". I've put them in this order intentionally. I will add comment about that.

yvvan marked an inline comment as done.May 25 2018, 12:27 AM
yvvan added inline comments.
lib/Sema/SemaCodeComplete.cpp
4115

I've rechecked this part - I can actually now move it without breaking anything

yvvan updated this revision to Diff 148553.May 25 2018, 12:29 AM

NIT-s addressed

ilya-biryukov accepted this revision.May 25 2018, 2:43 AM

Thanks for addressing the NITs. LGTM!

This revision is now accepted and ready to land.May 25 2018, 2:43 AM
This revision was automatically updated to reflect the committed changes.