search

sourcegraph / srclib

srclib is a polyglot code analysis library, built for hackability. It consists of language analysis toolchains (currently for Go and Java, with Python, JavaScript, and Ruby in beta) with a common output format, and a CLI tool for running the analysis.
https://srclib.org
Other
942 stars 62 forks source link

Parallise over units for unitStore ref filters #181

Closed keegancsmith closed 9 years ago

dmitshur commented 9 years ago

I've made a few style and code suggestions for possible improvements. Most are minor.

The only larger suggestion/comment I have is the way the parallelism is implemented, I think it would be nicer to do it more idiomatically by using a channel to collect the results rather than appending to a mutex-protected slice.

Otherwise I'm not spotting any other issues.

keegancsmith commented 9 years ago

The reason it was done with locks is the method implements a similiar thing above, and it used locks. I'll clean up this PR to make it more idiomatic go and address your suggestions.

dmitshur commented 9 years ago

That's completely understandable. The style suggestions can be a part of a separate followup PR; this PR can focus on functionality.

keegancsmith commented 9 years ago

I'm not a go expert so I am likely doing something noobish and non-idiomatic, but using channels instead of a simple lock makes the code much more complex in my view. So I'll stick for now with the mutex protected version - https://gist.github.com/keegancsmith/4d6bec9f64ba2f665f4f

dmitshur commented 9 years ago

I'm okay with that.

The diff looks good, and I don't see anything non-idiomatic about it, except the usage of parallel.NewRun maybe. The Go standard library, for example, would not do that, it would just use the sync.WaitGroup primitive directly.

Personally, the channel version looks beautiful and simple to my eyes, even though it's longer amount of code. But that's because I'm used to that kind of Go code and I know it's technically superior.

keegancsmith commented 9 years ago

Ok, I'll try craft some code that pleases your eyes :) /me is trying to level up his Go skills

keegancsmith commented 9 years ago

Actually I'll follow up with more idiomatic Go at a later stage. The more I think about it the more I prefer the terseness/KISS of the current version.

dmitshur commented 9 years ago

I think that's the best way to go. :) Leave it as is for now, but consider coming back to it at a later date. Refactoring is best done when it has a purpose and strong inner motivation.

sqs commented 9 years ago

@keegancsmith, is this ready to merge?