[PATCH] D33024: [Support] Move LLD parallel algorithms to LLVM.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: lld/COFF/Writer.cpp:748
       memset(SecBuf, 0xCC, Sec->getRawSize());
-    for_each(parallel::par, Sec->getChunks().begin(), Sec->getChunks().end(),
-             [&](Chunk *C) { C->writeTo(SecBuf); });
+    for_each(llvm::parallel::par, Sec->getChunks().begin(),
+             Sec->getChunks().end(), [&](Chunk *C) { C->writeTo(SecBuf); });
----------------
zturner wrote:
> chandlerc wrote:
> > I find the fact that `for_each` is now getting looked up via ADL on the `llvm::parallel::par` type ... disturbing.
> I can certainly qualify the function call, but I don't know of a way to disable ADL if that's what you're suggesting.
It's "easy": `foo(a, b, c)` uses ADL and `(foo)(a, b, c)` doesn't use ADL. ;]

I'm not suggesting using `(for_each)` though. That seems a bit crazy.

Writing `llvm::foo` will also disable ADL. ADL only fires for unqualified names. But then writing the code gets super ugly:

  llvm::for_each(llvm::parallel::par, ...);

Yuck. I guess I mildly prefer that? The standard sure chose an API that ends up requiring a lot of characters to type.


What do you or Rui think? I don't have strong feelings about which way you go here, so would generally defer to your or Rui's judgement.


================
Comment at: llvm/include/llvm/Support/TaskGroup.h:51-53
 /// \brief Allows launching a number of tasks and waiting for them to finish
 ///   either explicitly via sync() or implicitly on destruction.
 class TaskGroup {
----------------
zturner wrote:
> chandlerc wrote:
> > I thought all of this was being moved to be implementation details of the algorithms?
> > 
> > Also, all the code appears to still be LLD style?
> The executor stuff was all moved to an anonymous namespace inside of `TaskGroup.cpp`.  `TaskGroup` cannot enjoy the same level of hiding because it needs to be usable from within a header file.  Namely, `Parallel.h`.  I can, however, move it to a detail namespace inside of `Parallel.h` if you prefer that.
> 
> also: Yes rui pointed out the names in a previous patch and I forgot to update them.  All of them have been fixed except for the few instances in this file.
I see, thanks.

I think a detail namespace is a touch cleaner. I'm happy for you to do that in this patch, same with any name cleanup.


https://reviews.llvm.org/D33024