Page MenuHomePhabricator

[clang] Use CompilerInstance::createTarget to createTarget
AbandonedPublic

Authored by oToToT on Feb 26 2021, 8:40 AM.

Details

Summary

As proposed in D97109, I tried to replace all similar target creating logics in clang to a single CompilerInstance::createTarget call introduced in D97493.

This improve the consistency while creating Target.

Diff Detail

Event Timeline

oToToT requested review of this revision.Feb 26 2021, 8:40 AM
oToToT created this revision.
oToToT edited the summary of this revision. (Show Details)Feb 26 2021, 8:40 AM
oToToT added inline comments.Feb 26 2021, 8:45 AM
clang/lib/Frontend/PrecompiledPreamble.cpp
368

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?

oToToT added a comment.Mar 1 2021, 7:15 PM

Kindly ping.

Since the part I modified is quite... old (about 4 to 7 years ago?), I hope I've found the right person to review for this.

sammccall added inline comments.Mar 2 2021, 4:39 AM
clang/lib/Frontend/ASTUnit.cpp
1153

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)

clang/lib/Frontend/ChainedIncludesSource.cpp
152

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.

clang/lib/Frontend/PrecompiledPreamble.cpp
368

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.

oToToT abandoned this revision.Mar 6 2021, 1:11 PM

Since, originally, I think it is OK to submit patch just by project, but I agree that it's better to make patch as minimal as possible.
Thus, I will abandon this and resubmit another patch.
(I think reuploading patch and changing the whole patch logic here might not be great?)

clang/lib/Frontend/PrecompiledPreamble.cpp
368

Yes, this is because the inconsistency between preamble and main AST. I will reorder my patch.