This is an archive of the discontinued LLVM Phabricator instance.

[clang][Tooling] Optimize addTargetAndMode in case of invalid modes
ClosedPublic

Authored by kadircet on Aug 1 2020, 10:04 AM.

Details

Summary

This skips searching for target related flags in the existing args if
we don't have a valid target to insert.

Depends on D85076

Diff Detail

Event Timeline

kadircet created this revision.Aug 1 2020, 10:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2020, 10:04 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kadircet requested review of this revision.Aug 1 2020, 10:04 AM
hokein added inline comments.Aug 3 2020, 12:26 AM
clang/lib/Tooling/Tooling.cpp
263

While this is an optimization, I find the code is a bit harder to follow, with this patch AlreadyHasTarget has two semantic meanings: 1) for has target and 2) for target mode is valid.

I guess we could do it like

if (TargetMode.TargetIsValid) {
  // set the TargetOPT, TargetOPTLegacy variables
  // search the command line of the target opt.
  // insert to CommandLine.
}

maybe we could do the same thing for the DriverMode

if (TargetMode.DriverMode) {
  ...
}
kadircet added inline comments.Aug 3 2020, 2:38 AM
clang/lib/Tooling/Tooling.cpp
263

i would rather not duplicate the loop, what about renaming AlreadyHastTarget to ShouldAddTarget (likewise for AlyreadyHasMode) ?

hokein added inline comments.Aug 3 2020, 2:47 AM
clang/lib/Tooling/Tooling.cpp
263

I'd not worry too much about the loop, but rename also seems ok to me.

kadircet updated this revision to Diff 282557.Aug 3 2020, 2:58 AM
  • Rename AlreadyHas to ShouldAdd (and revert the logic)
kadircet marked an inline comment as done.Aug 3 2020, 2:58 AM
kadircet marked an inline comment as done.
hokein accepted this revision.Aug 3 2020, 4:27 AM
This revision is now accepted and ready to land.Aug 3 2020, 4:27 AM
kadircet updated this revision to Diff 282584.Aug 3 2020, 4:51 AM
  • Change bitwise assignment to logical operators, as bitwise operators do not have short-circuting.
This revision was landed with ongoing or failed builds.Aug 3 2020, 5:01 AM
This revision was automatically updated to reflect the committed changes.