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

Gather all names before printing them. #17

Closed
wants to merge 2 commits into from

Conversation

fdevibe
Copy link
Contributor

@fdevibe fdevibe commented Mar 22, 2016

When issuing a NAMES request, the reponse is chunked. Cache names
until all are received, then print them out.

When issuing a NAMES request, the reponse is chunked. Cache names
until all are received, then print them out.
@fdevibe
Copy link
Contributor Author

fdevibe commented Mar 22, 2016

My Go experience is pretty much limited to my patches to this project. My array / slice handling here seems somewhat dysfunctional to me, any hints on how to improve this (or just fixing it) would be much appreciated.

@@ -25,6 +27,7 @@ func NewBridge(name string, config *Config) *Bridge {
b.Config = config
b.cmap = make(map[string]string)
b.ircNick = b.Config.IRC.Nick
b.names = make([]string, 0, 10000)
Copy link
Owner

Choose a reason for hiding this comment

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

This is not necessary for our use case, we don't need to allocate this much. We can just use the append function to grow dynamically. (line can be removed)

@42wim
Copy link
Owner

42wim commented Mar 22, 2016

Looks pretty good, I'm no expert myself though.
Gave you some notes how I would change those 2 lines :-)
Something particular you're not sure about?

@fdevibe
Copy link
Contributor Author

fdevibe commented Mar 22, 2016

@42wim thanks for the input. From the Go docs, it wasn't clear to me it could be done this way. Pushed now. :-)

@42wim 42wim closed this Mar 24, 2016
42wim added a commit that referenced this pull request Apr 11, 2022
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