[PATCH] D22518: Refactor how include paths are appended to the command arguments.

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: lib/Driver/Tools.cpp:308-335
@@ -349,1 +307,30 @@
+    Action::OffloadKind ActiveOffloadingKind = Action::OFK_None) {
+  SmallVector<const ToolChain *, 3> RelevantToolChains;
+  // Add the current tool chain to the relevant tool chain list if it is
+  // defined.
+  if (RegularToolChain)
+    RelevantToolChains.push_back(RegularToolChain);
+
+  // Add all the offloading tool chains associated with the current action to
+  // the relevant tool chain list. If we don't have a specific active offload
+  // kind, consider all available, otherwise consider only the active kind.
+  if (ActiveOffloadingKind == Action::OFK_None ||
+      ActiveOffloadingKind == Action::OFK_Cuda) {
+    if (JA.isHostOffloading(Action::OFK_Cuda))
+      RelevantToolChains.push_back(
+          C.getSingleOffloadToolChain<Action::OFK_Host>());
+    else if (JA.isDeviceOffloading(Action::OFK_Cuda))
+      RelevantToolChains.push_back(
+          C.getSingleOffloadToolChain<Action::OFK_Cuda>());
+  }
+
+  //
+  // TODO: Add support for other offloading programming models here.
+  //
+
+  // Apply Work on all the relevant tool chains.
+  for (const auto *TC : RelevantToolChains) {
+    assert(TC && "Undefined tool chain??");
+    Work(TC);
+  }
 }
----------------
There's no point in building a `SmallVector` here, just directly call `Work` when you find a toolchain that should be used.

================
Comment at: lib/Driver/Tools.cpp:317-318
@@ +316,4 @@
+  // kind, consider all available, otherwise consider only the active kind.
+  if (ActiveOffloadingKind == Action::OFK_None ||
+      ActiveOffloadingKind == Action::OFK_Cuda) {
+    if (JA.isHostOffloading(Action::OFK_Cuda))
----------------
What is this `ActiveOffloadingKind` parameter for? Both values that we actually pass in here do the exact same thing.

================
Comment at: lib/Driver/Tools.cpp:341-346
@@ -340,8 @@
-
-  if (JA.isHostOffloading(Action::OFK_Cuda))
-    C.getSingleOffloadToolChain<Action::OFK_Host>()->AddCudaIncludeArgs(
-        Args, CmdArgs);
-  else if (JA.isDeviceOffloading(Action::OFK_Cuda))
-    C.getSingleOffloadToolChain<Action::OFK_Cuda>()->AddCudaIncludeArgs(
-        Args, CmdArgs);
-
----------------
The OFK_Host / OFK_Cuda arguments here are reversed from the other two cases. Is that a bug that's fixed by this change, or a bug that's introduced by this change? :)

Either way it seems that we're missing test coverage.


https://reviews.llvm.org/D22518