[PATCH] D142905: [Driver] Change multilib selection algorithm

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added inline comments.


================
Comment at: clang/include/clang/Driver/Multilib.h:41
+  flag_set Flags;
+  arg_list PrintArgs;
 
----------------
phosek wrote:
> This is just a suggestion, but GCC documentation refers to these as either "options" or "switches", not "args" so I think it might be helpful to use the same nomenclature. This would also avoid confusion since the term "args" is used extensively throughout the driver but refers to input arguments.
OK, I've gone with "options" just because `switch_list` would be an unintuitive name.


================
Comment at: clang/lib/Driver/MultilibBuilder.cpp:90-93
+  for (StringRef Flag : Flags) {
+    if (Flag.front() == '+')
+      Args.push_back(("-" + Flag.substr(1)).str());
+  }
----------------
phosek wrote:
> If I understand this correctly, we might end up with duplicate arguments in the case when `Flags` contains duplicate elements. Is that desirable? Wouldn't it be better to construct the list of arguments from the set of fags inside `Multilib`?
Yes, you can have duplicate options if you write code to do that. The Multilib class has never previously had any functionality to remove duplicate options so I'd rather not change that. Previously if you wrote code that added the same option twice then `clang -print-multi-lib` would reflect that. This behaviour remains unchanged.

No, you can't construct the list of options from the set of flags inside `Multilib` because `Multilib`'s flags are not command line options. (They will typically look similar to command line options but there's no requirement for that to be the case). I've added comments here and for the flags() method to hopefully make this clearer. Hopefully the docs in D143587 will also make things clearer for anyone else trying to reason about the multilib system.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142905