[PATCH] D97561: [clang] Use CompilerInstance::createTarget to createTarget

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/lib/Frontend/ASTUnit.cpp:1153
   // Create the target instance.
-  Clang->setTarget(TargetInfo::CreateTargetInfo(
-      Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
----------------
This part is not used by clangd. However it is used together with PrecompiledPreamble, so maybe all three need to be changed together.

(Just trying to understand whether this is part of the minimal change, but I think you have this right)



================
Comment at: clang/lib/Frontend/ChainedIncludesSource.cpp:152
     Clang->setDiagnostics(Diags.get());
-    Clang->setTarget(TargetInfo::CreateTargetInfo(
-        Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
+    Clang->createTarget();
     Clang->createFileManager();
----------------
This looks like a good, correct change, but I think it should be in a separate patch.
This affects the behavior of clang itself, which means:
 - it's riskier to change
 - keeping the inconsistency for a little while longer can't be too bad, because it's long-standing behavior

I suspect `-Xclang -chained-include` is just really rare in practice.


================
Comment at: clang/lib/Frontend/PrecompiledPreamble.cpp:368
   // Create the target instance.
-  Clang->setTarget(TargetInfo::CreateTargetInfo(
-      Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
-  if (!Clang->hasTarget())
+  if (!Clang->createTarget())
     return BuildPreambleError::CouldntCreateTargetInfo;
----------------
oToToT wrote:
> Changing this without other further patch might cause `clangd` results in Runtime Error while handling files like CUDA.
> Should I also include the patch of `clangd` or other related projects into this patch?
What kind of runtime error can this result in and why? What's the current behavior?

My guess is if this has to be consistent between preamble + main AST, then you should change PrecompiledPreamble, ASTUnit, and clangd together in one patch and leave others.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97561