Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Oct 30, 2018
Merged

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

merged 4 commits into from
Oct 30, 2018

Conversation

3246251196
Copy link
Contributor

…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.

…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
Copy link
Owner

Sarcasm commented Oct 26, 2018

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
Copy link
Contributor Author

3246251196 commented Oct 28, 2018

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 added not-accessible' to the filtered candidates.

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

@Sarcasm
Copy link
Owner

Sarcasm commented Oct 28, 2018

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
Copy link
Contributor Author

@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.

Copy link
Owner

@Sarcasm Sarcasm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for the explanation, make sense to me now.

Would be nice to explain this situation in the filtering code.

(memq (irony-completion-availability candidate)
irony-completion-availability-filter))
candidates))
(let (unique-candidates)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe (cons 1 (cons 2 (cons 3 nil))) is the same as (list 1 2 3).

I think the first when is useless now, as it is no longer the final statement.

Maybe you should use (and (memq ...) (push unique-key ...).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, I meant to rewrite the code like this:

  (and (memq (irony-completion-availability candidate)
		   irony-completion-availability-filter))
	 (let ((unique-key (list (irony-completion-typed-text candidate)
                                 (irony-completion-annotation candidate)
                                 (irony-completion-type candidate))))
	   (and (not (member unique-key unique-candidates))
                (push unique-key unique-candidates)))))

2 when, makes the first useless:

(defun (a b)
  (when a t)
  (when b t))

Here, whatever the value of a, t will be returned only if b is non-nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, just got back from work.

Yes, that makes sense.

@3246251196
Copy link
Contributor Author

@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
Copy link
Owner

Sarcasm commented Oct 29, 2018

Agreed for the option.

@3246251196
Copy link
Contributor Author

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 Sarcasm merged commit 0a5ea0b into Sarcasm:master Oct 30, 2018
@Sarcasm
Copy link
Owner

Sarcasm commented Oct 30, 2018

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants