This is an archive of the discontinued LLVM Phabricator instance.

Fix overloaded static functions for templates
ClosedPublic

Authored by yvvan on Feb 19 2018, 2:41 AM.

Diff Detail

Repository
rC Clang

Event Timeline

yvvan created this revision.Feb 19 2018, 2:41 AM

I need now to extract the duplicated part and make some test for template...

yvvan updated this revision to Diff 134884.Feb 19 2018, 3:56 AM

Clean up code and add tests

yvvan updated this revision to Diff 134886.Feb 19 2018, 4:09 AM

Fix typo in comment

yvvan updated this revision to Diff 136695.Mar 2 2018, 4:23 AM

Fix one of AddMethodTemplateCandidate parameters.
Tests pass now

Here's the file we use in our parameter info tests locally (that don't pass with the trunk of clang, but with your changes should now theoretically pass):

class B
{
public:
	/// B ctor
	B() {
		priv();
		spriv();
		pub();
		spub();
		o();
		this->priv();
		this->spriv();
		this->pub();
		this->spub();
		this->o();
		B::priv();
		B::spriv();
		B::pub();
		B::spub();
		B::o();
		this->B::priv();
		this->B::spriv();
		this->B::pub();
		this->B::spub();
		this->B::o();
	}

	/// a
	void pub();
	/// b
	static void spub();
	/// o
	void o();

private:
	/// c
	void priv();
	/// d
	static void spriv();
};

class C : public B
{
public:
	/// e
	virtual void cpub() {
		cpriv();
		cspriv();
		cpub();
		cspub();
		o();
		o(1);
		o(0.5f);
		o(true);
		this->cpriv();
		this->cspriv();
		this->cpub();
		this->cspub();
		this->o();
		C::cpriv();
		C::cspriv();
		C::cpub();
		C::cspub();
		C::o();
		C::o(1);
		C::o(0.5f);
		C::o(true);
		this->C::cpriv();
		this->C::cspriv();
		this->C::cpub();
		this->C::cspub();
		this->C::o();
		this->C::o(1);
		this->C::o(0.5f);
		this->C::o(true);

		// base calls
		pub();
		spub();
		this->pub();
		this->spub();
		B::pub();
		B::spub();
		B::o();
		this->B::pub();
		this->B::spub();
		this->B::o();

		B();

		// templates
		tfoo<int>();
		stfoo<float>();
		dfoo<double>();
		dfoo(3.5);
		dfoo2(1);
		sdfoo2(false);
		this->tfoo<int>();
		this->stfoo<float>();
		this->dfoo<double>();
		this->dfoo(3.5);
		this->dfoo2(1);
		this->sdfoo2(false);
		C::stfoo<decltype(nullptr)>();
		C::sdfoo2(true);

		// templates without type arguments (potential deduction)
		dfoo();
		dfoo2();
		sdfoo2();
		this->dfoo();
		this->dfoo2();
		this->sdfoo2();
		C::dfoo();
		C::dfoo2();
		C::sdfoo2();
		this->C::dfoo();
		this->C::dfoo2();
		this->C::sdfoo2();

		// other special cases
		(B::pub)();
		(C::pub)();
		(B::o)();
		(C::o)();
	}
	/// f
	static void cspub();
	/// o override
	void o();
	/// overload0
	void o(int);
	/// overload1
	void o(float);

private:
	/// g
	void cpriv();
	/// h
	static void cspriv();

	/// simple template
	template<typename T> void tfoo();

	/// simple static template
	template<typename T> static void stfoo();

	/// template with default param
	template<typename T> void dfoo(int x = 99-22);

	/// template with deduced param
	template<typename T> void dfoo(T x);

	/// template with deduced param and default param
	template<typename T> void dfoo2(T x = 0);

	/// static template with deduced param and default param
	template<typename T> static inline void sdfoo2(T x = 0) {
};
yvvan added a comment.Mar 19 2018, 3:52 AM

@cameron314 yes, these should be covered now

Review ping

ioeric added inline comments.Jun 19 2018, 3:41 AM
lib/Sema/SemaOverload.cpp
6378

This seems to assume that D is either FunctionDecl or template? Could we be more explicit here and use dyn_cast<FunctionTemplateDecl>?

6403

Could you add a brief comment here explaining that this branch handles both standalone functions and static member functions?

yvvan added a comment.Jun 19 2018, 3:55 AM

Thanks for the feedback :) I will return to this one and address comments a little bit later.

yvvan updated this revision to Diff 152234.Jun 21 2018, 1:01 AM
yvvan marked 2 inline comments as done.

comments addressed

Looks good. Where did the tests go? ;)

ioeric added inline comments.Jun 21 2018, 1:08 AM
lib/Sema/SemaOverload.cpp
6376

IsTemplate seems redundant. You could use FunTmpl in the conditions below I think? This avoids maintaining two state variables.

yvvan updated this revision to Diff 152235.Jun 21 2018, 1:11 AM

tests are back :)

yvvan added inline comments.Jun 21 2018, 1:12 AM
lib/Sema/SemaOverload.cpp
6376

I think bool value is more descriptive in this case

ioeric accepted this revision.Jun 21 2018, 1:25 AM

lgtm

lib/Sema/SemaOverload.cpp
6376

I think FunImpl is clear enough. But up to you.

Also this could be const bool IsTemplate = FunTmpl != nullptr;

This revision is now accepted and ready to land.Jun 21 2018, 1:25 AM
yvvan updated this revision to Diff 152236.Jun 21 2018, 1:31 AM

Ok, I reconsidered, let's remove IsTemplate :)

This revision was automatically updated to reflect the committed changes.