[PATCH] D146757: [Driver] Enable defining multilib print options independently of flags

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Approach looks good to me. Some suggestions, mostly around documenting the interface.



================
Comment at: clang/include/clang/Driver/Multilib.h:56
   /// This is enforced with an assert in the constructor.
   Multilib(StringRef GCCSuffix = {}, StringRef OSSuffix = {},
+           StringRef IncludeSuffix = {}, const flags_list &Flags = flags_list(),
----------------
I think it is worth adding some information from the commit message to the comment. In particular what the default behaviour is and what, if any, restrictions are there on the PrintOptionsList.

For example there is a comment in the  assert in the constructor body that probably ought to be in the header
```
// If PrintOptions is something other than List then PrintOptionsList must be
// empty.
```

Are there any restrictions on the format of PrintOptionsList for it to print correctly? For example does each option need to be prefixed with `-`? 


================
Comment at: clang/include/clang/Driver/Multilib.h:58
+           StringRef IncludeSuffix = {}, const flags_list &Flags = flags_list(),
+           PrintOptionsType PrintOptions = PrintOptionsType::PlusFlags,
+           const flags_list &PrintOptionsList = flags_list());
----------------
How would someone using `makeMultilib()` from `MultilibBuilder` use these parameters? If they can't do they need to? If so we may need something like a makeMultilib overload to supply the extra parameters.


================
Comment at: clang/lib/Driver/Multilib.cpp:44
+  assert(PrintOptions == PrintOptionsType::List || PrintOptionsList.empty());
+}
+
----------------
If there are any restrictions on the strings in PrintOptionsList could be worth assertions. Please ignore if there are no restrictions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146757