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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu 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:
> > 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.
> It's worse than that, because it's going to be `llvm::parallel::for_each(llvm::parallel::par, ...)`.  We could nuke the intermediate `parallel` namespace and go with `llvm::for_each(llvm::par, ...)`, or we could do `using llvm::parallel` at the top of a file.  The namespace is sufficiently scoped that bringing it in shouldn't be too onerous.
> 
> Personally I'm fine with any approach, including the ADL-based approach.  That said, I don't think there's anything we can do to the API itself to alleviate this, so perhaps we don't need to do anything.  Just leave it up to the users.
Because we have `using namespace llvm` at beginning of this file, you don't need to qualify it with `llvm::`, no?


https://reviews.llvm.org/D33024