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

allow NewPoolWithFunc can invoke with nil argument #167

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bingoohuang
Copy link

  1. abstract a Task interface for SubmitTask which is more convenient

name: Pull request
about: Propose changes to the code
title: 'allow NewPoolWithFunc can invoke with nil argument'
labels: ''
assignees: ''

1. Are you opening this pull request for bug-fixs, optimizations or new feature?

optimizations

2. Please describe how these code changes achieve your intention.

  1. allow NewPoolWithFunc can invoke with nil argument
  2. abstract a Task interface for SubmitTask which is more convenient

3. Please link to the relevant issues (if any).

none

4. Which documentation changes (if any) need to be made/updated because of this PR?

none

4. Checklist

  • I have squashed all insignificant commits.
  • I have commented my code for explaining package types, values, functions, and non-obvious lines.
  • I have written unit tests and verified that all tests passes (if needed).
  • I have documented feature info on the README (only when this PR is adding a new feature).
  • (optional) I am willing to help maintain this change if there are issues with it later.

2. abstract a Task interface for SubmitTask which is more convenient
@ioworker0
Copy link
Contributor

This PR could introduce a problem.

// The address of two struct{} values may be the same.

var a, b struct{}
fmt.Println(&a == &b) // true

My case

package main

import (
	"fmt"
)

var stopArg interface{} = &struct{}{}
var stopArg1 interface{} = &struct{}{}
var stopArg2 interface{} = &struct{}{}

func main() {
	check(stopArg1)
	check(stopArg2)
}

func check(iface interface{}) {
	if iface == stopArg {
		fmt.Println("RETURN")
		return
	}
	fmt.Println("RUN")
}

OUTPUT

RETURN
RETURN

[Process exited 0]

I'm sorry that I offended you. That wasn't my intent.

@ioworker0
Copy link
Contributor

@bingoohuang

@bingoohuang
Copy link
Author

bingoohuang commented Jun 15, 2021

@Mutated1994 thanks for your pointing out the potential bug for the stopArg definition

I fixed to var stopArg interface{} = &struct{ a byte }{} and the test cases like

func TestAntsPoolWithFuncNilParam(t *testing.T) {
	var old1 interface{} = &struct{ a byte }{}
	var old2 interface{} = &struct{ a byte }{}
	assert.True(t, old1 != old2)

	isStopArg := func(v interface{}) bool { return v == stopArg }

	var a1 interface{} = &struct{ a byte }{}
	var a2 interface{} = &struct{ a byte }{}
	var a3 interface{} = &struct{ a byte }{}
	assert.False(t, a1 == a2)
	assert.False(t, isStopArg(a1))
	assert.False(t, isStopArg(a2))
	assert.False(t, isStopArg(a3))
	assert.True(t, isStopArg(stopArg))
}

please have a code view for the last change, thanks very much.

@ioworker0
Copy link
Contributor

@Mutated1994 thanks for your pointing out the potential bug for the stopArg definition

I fixed to var stopArg interface{} = &struct{ a byte }{} and the test cases like

func TestAntsPoolWithFuncNilParam(t *testing.T) {
	var old1 interface{} = &struct{ a byte }{}
	var old2 interface{} = &struct{ a byte }{}
	assert.True(t, old1 != old2)

	isStopArg := func(v interface{}) bool { return v == stopArg }

	var a1 interface{} = &struct{ a byte }{}
	var a2 interface{} = &struct{ a byte }{}
	var a3 interface{} = &struct{ a byte }{}
	assert.False(t, a1 == a2)
	assert.False(t, isStopArg(a1))
	assert.False(t, isStopArg(a2))
	assert.False(t, isStopArg(a3))
	assert.True(t, isStopArg(stopArg))
}

please have a code view for the last change, thanks very much.

Dude, it's my pleasure.

LGTM

@panjf2000
Copy link
Owner

Why does ants need this?

@bingoohuang
Copy link
Author

manbe you can ask, why does not ants support this.

There is no need for ants to limit users to pass a nil argument to their register function as some special meaning.

@panjf2000
Copy link
Owner

panjf2000 commented Jun 20, 2021

Just because the fact that we can do, doesn't mean we should do.

I'm afraid you'll have to prove that this PR is necessary for users, BTW, I also believe there are ways to achieve the same effect based on the existing APIs.

@ioworker0
Copy link
Contributor

Just because the fact that we can do, doesn't mean we should do.

I'm afraid you'll have to prove that this PR is necessary for users, BTW, I also believe there are ways to achieve the same effect based on the existing APIs.

TBH, I couldn't agree more.

@bingoohuang
Copy link
Author

Just because the fact that we can do, doesn't mean we should do.

I'm afraid you'll have to prove that this PR is necessary for users, BTW, I also believe there are ways to achieve the same effect based on the existing APIs.

Yeah, I agree with you, but it's the user's choice to make a decision to do or not to do, not the common library or the language itself.

TLDR, It's a user's duty, not the ants'.

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.

None yet

3 participants