[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