[PATCH] D97109: [clangd] Add support for auxiliary triple specification

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 23 02:10:47 PST 2021


sammccall added a comment.

Thanks for working this out!

Clearly there are some layering concerns here - clangd is "just" a clang tool that parses some code, and this is some pretty hairy clang internal logic we're copying here.
Practical concerns:

- it it could very easily skew from clang (e.g. will Sycl + CUDA + OpenMP always be the right list?!)
- there are other tools (callers of setTarget) that also need this logic
- this isn't even all the target logic we're missing, there's also fpenv stuff in CompilerInstance::ExecuteAction

Moreover this is entirely propagating settings from CompilerInvocation (spec) to CompilerInstance (state), which naturally belongs in ComplilerInstance.

I'd suggest this plan:

1. extract the target-related logic in CompilerInstance::ExecuteAction to a method createTarget(). This is NFC
2. update clangd + PrecompiledPreamble to call createTarget() instead of setTarget() +getTarget()->adjust() etc. This fixes this bug and others in clangd.
3. (optional, for you or me or others) update other callers of setTarget() in a similar way, which simplifies them and fixes bugs there too.

WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97109



More information about the cfe-commits mailing list