Sarcasm / irony-mode

A C/C++ minor mode for Emacs powered by libclang
GNU General Public License v3.0
901 stars 98 forks source link

Do not display duplicate completions. As well as checking that the av… #503

Closed 3246251196 closed 5 years ago

3246251196 commented 5 years ago

…ailability of the candidate is in the user's allowed availability filtered list - only add the element if it is unique. The current key used is a tuple of typed-text, annotation and return type.

==== @Sarcasm , I just wanted to get the ball rolling on duplicate completions. All the completions are strictly unique, but - at least for company-irony - the things that are displayed are the typed text, signature and return type. I use irony-mode at work on a large code base and see a lot of third party library duplicates.

Can you have a look at this pull-request so we can discuss how to remove duplicates. One issue I can think of even now is that we may be removing a duplicate that has useful brief documentation.

On a side/slightly related note: the new version of clan tools (cfe) has a recent fix to the protected member issue (if you remember it); this means we can soon remove the availability filtering - though we may want to cater for people using older version of clang. Furthermore, I may be the only one actually using/modifying `irony-completion-availability-filter' from its default value anyway!

Cheers.

Sarcasm commented 5 years ago

Do you have an exemple of what is deduplicated with your code? Is it something we want for everyone on by default, or is it up to debate?

For example, does this deduplicate const and non-const getters?

const std::string & name() const;
std::string & name();
3246251196 commented 5 years ago

Do you have an exemple of what is deduplicated with your code? Is it something we want for everyone on by default, or is it up to debate?

For example, does this deduplicate const and non-const getters?

const std::string & name() const;
std::string & name();

Currently I am testing with:

#include <string>

struct MyClass0
{
    MyClass0( void ) { }
    ~MyClass0( void ) { }
    virtual int theFunction( float x ) = 0;
    const std::string thisFunc( void ) const { }
    std::string thisFunc( void ) { }

    virtual const float overrideMe(void){}
    virtual float overrideMe(void) const {}

};

struct MyClass1 : public MyClass0
{
    MyClass1( void ) : MyClass0() { }
    virtual ~MyClass1( void ) { }

    virtual int theFunction( float x ) override { (void)x; }
    virtual const float overrideMe(void) override { ; } 
protected:
    int x;
};

struct MyClass2 : public MyClass1
{
    MyClass2( void ) : MyClass1() { }
    ~MyClass2( void ) { }

    virtual int theFunction( float x ) override { (void)x; }

    void someOtherFn()
        {
        }

    virtual float overrideMe( void ) const { ; }
};

int main( void )
{
    MyClass2 my_class_2;
    my_class_2.
    return 0;
}

Completion results: image

(notice that x' is available because I have addednot-accessible' to the filtered candidates.

I think it would be safest to make this an optional thing.

Sarcasm commented 5 years ago

I use irony-mode at work on a large code base and see a lot of third party library duplicates.

Do you have a concrete example of this? I'm still not sure what is duplicated.

Did any candidate was removed in the example you just showed? If yes, which one(s)?

3246251196 commented 5 years ago

@Sarcasm So all virtual methods show up in the list - even though they show the same output. If I have three classes that derive from each other with a virtual method and each class overrides it I will see three duplicate entries. Irony-mode today shows the following results:

image

And, at work with my huge code base including third party libraries there are many more duplicates.

3246251196 commented 5 years ago

@Sarcasm

I am just leaving from work now and made some quick changes as per your comment. Can you let me know why we should remove the first when - I only want to bother duplicates if the candidate passes the availability filter. Also, I have to use member since we have to compare string values - memq tests whether the actual objects are the same.

Apart from the above comments, we should consider whether we want this to be an option or not: I would vote for yes.

Thanks.

Sarcasm commented 5 years ago

Agreed for the option.

3246251196 commented 5 years ago

So I have added a customizable bool option for this.

Just to let you know that given the following:

struct C0
{
  /// \brief The Base Method
  virtual void overrideMe( void ) { }
};

struct C1 : public struct C0
{
  /// \brief The Derived Method
  virtual void overrideMe (void) override {}
};

int main(void)
{
  C1 someInstance;
  someInstance.//candidates here
}

This means that the brief/doxygen is arbitrarily the first one it comes across - the message in the minibuffer.

Cheers.

Sarcasm commented 5 years ago

I have to admit that I'm a bit lost with all the and, or, not, but let's try this and if some conditions aren't right, or if there is a better way to write this, it can be done later.

Thanks for the PR!