peter.smith added a comment.

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

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 = 
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 = 
+           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.

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to