This is an archive of the discontinued LLVM Phabricator instance.

[python] [tests] Update test_code_completion
ClosedPublic

Authored by mgorny on Aug 2 2018, 1:30 AM.

Details

Summary

Update expected completions to match output generated by clang-7.0.

Disclaimer: I have no clue if the results are correct. I'm kinda wondering why the '::' part is no longer included immediately.

Also PR38417.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny created this revision.Aug 2 2018, 1:30 AM

Also can't explain why const and volatile have a different priority now.
The P:: and Q:: seem to be completely different completion items from P and Q (wildly different priorities suggest they're not the same), can't explain what caused the first ones not being added anymore.

Happy to LGTM to unbreak tests, but won't have time to dig up the commits that introduced these changes.

PS Could you upload the diff with full context?

mgorny updated this revision to Diff 164871.Sep 11 2018, 7:11 AM

Ai, sorry about that. Uploaded the proper diff now. I suppose it's not going to make it for 7.0.0 anymore, so it's not a priority. I'll try to bisect it today once I finish testing RC3.

Apparently the first change changing the completion results is:

commit a408f8d27bfd5bab55c39ef2a6fff6850be4a351
Author: Ilya Biryukov <ibiryukov@google.com>
Date:   Tue Apr 24 15:48:53 2018

    [CodeComplete] Fix completion at the end of keywords
    
    Summary:
    Make completion behave consistently no matter if it is run at the
    start, in the middle or at the end of an identifier that happens to
    be a keyword or a macro name. Since completion is often ran on
    incomplete identifiers, they may turn into keywords by accident.
    
    For example, we should produce same results for all of these
    completion points:
    
        // ^ is completion point.
        ^class
        cla^ss
        class^
    
    Previously clang produced different results for the last case (as if
    the completion point was after a space: `class ^`).
    
    This change also updates some offsets in tests that (unintentionally?)
    relied on the old behavior.
    
    Reviewers: sammccall, bkramer, arphaman, aaron.ballman
    
    Reviewed By: sammccall
    
    Subscribers: cfe-commits
    
    Differential Revision: https://reviews.llvm.org/D45887
    
    git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@330717 91177308-0d34-0410-b5e6-96231b3b80d8

...and unless I'm mistaken, this commit is responsible for both changes (that is, I need to apply all of this patch to make tests pass at its point).

Will take a closer look. Thanks for finding the offending revision.

@ilya-biryukov, gentle ping. I'd like to patch this for 7.0.0 in Gentoo. Do you think my patch would be good enough, or do you expect to submit something else soonish?

ilya-biryukov accepted this revision.Sep 24 2018, 9:04 AM

@ilya-biryukov, gentle ping. I'd like to patch this for 7.0.0 in Gentoo. Do you think my patch would be good enough, or do you expect to submit something else soonish?

Sorry about the delay.
LGTM to allow committing/cherrypicking. I'll make sure to investigate why the change was there and explain/fix this case and update this thread.

This revision is now accepted and ready to land.Sep 24 2018, 9:04 AM
This revision was automatically updated to reflect the committed changes.