[PATCH] D116492: [lld] Deprecate using llvm-config to detect llvm installation

Mon Jan 3 10:23:30 PST 2022

Ericson2314 marked 2 inline comments as done.
Ericson2314 added inline comments.

================
Comment at: clang/CMakeLists.txt:26
       "--src-root"
-      "--cmakedir")
+      "--cmakedir"
+      "--bindir"
----------------
beanz wrote:
> Ericson2314 wrote:
> > beanz wrote:
> > > I assume these are re-arranged to match what you're doing in lld. Is that correct?
> > > 
> > > Generally it is preferred to do this kind of non-functional restructuring in a separate commit from the commit with a functional change to make it easier to review the functional changes.
> > > 
> > > This patch as-is seems to have cleanup to clang, and a functional change for lld. Those should likely be two separate commits.
> > Happy to split up.
> > 
> > The clang changes I wouldn't even say make it "better" per say, I am just reording things to put the stuff LLD also goes first for sake diffing the two. I thought that might look lke a noisy and pointless patch on its own, but I am happy to split it up if you still think it's a good idea.
> More smaller patches is generally the LLVM philosophy, so I do think it makes sense to split it up into an NFC patch for clang, and a separate change for lld.
Sure. Done in D116492.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116492/new/

https://reviews.llvm.org/D116492