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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
