[PATCH] D83694: [clang][cli] Port DependencyOutput option flags to new option parsing system

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/include/clang/Driver/Options.td:473
+    MarshallingInfoFlag<"DependencyOutputOpts.OutputFormat", "DependencyOutputFormat::Make">,
+    Normalizer<"makeFlagToValueNormalizer(DependencyOutputFormat::NMake)">;
 def Mach : Flag<["-"], "Mach">, Group<Link_Group>;
----------------
jansvoboda11 wrote:
> The original patch used the following normalizer:
> `(normalizeFlagToValue<DependencyOutputFormat, DependencyOutputFormat::NMake>)`.
> 
> I wanted to remove the parenthesis and redundant type argument, hence `makeFlagToValueNormalizer`. This could be useful later on when normalizing flags to non-literal types.
This seems a lot cleaner, thanks!


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:137-143
+template <typename T>
 void denormalizeSimpleFlag(SmallVectorImpl<const char *> &Args,
                            const char *Spelling,
                            CompilerInvocation::StringAllocator SA,
-                           unsigned TableIndex, unsigned Value) {
+                           unsigned TableIndex, T Value) {
   Args.push_back(Spelling);
 }
----------------
On the switch from `T` to `unsigned`, it'd be nice to avoid a separate instantiation for each enumeration, since the code is identical... it'll just bloat compile time for no reason. Maybe:
```
/// The tblgen-erated code passes in a fifth parameter of an arbitrary type, but
/// denormalizeSimpleFlags never looks at it. Avoid bloating compile-time with
/// unnecessary template instantiations and just ignore it with a variadic
/// argument.
static void denormalizeSimpleFlag(
    SmallVectorImpl<const char *> &Args, const char *Spelling,
    CompilerInvocation::StringAllocator, unsigned, /*T*/...) {
  Args.push_back(Spelling);
}
```
Note that it'd also be nice to make this `static` and drop unused parameter names, as I've done here.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:145-154
+template <typename T> struct FlagToValueNormalizer {
+  T Value;
+
+  llvm::Optional<T> operator()(OptSpecifier Opt, unsigned TableIndex,
+                               const ArgList &Args, DiagnosticsEngine &Diags) {
+    if (Args.hasArg(Opt))
+      return Value;
----------------
Please put this in an anonymous namespace.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:148-149
+
+  llvm::Optional<T> operator()(OptSpecifier Opt, unsigned TableIndex,
+                               const ArgList &Args, DiagnosticsEngine &Diags) {
+    if (Args.hasArg(Opt))
----------------
Please drop the parameter names for `TableIndex` and `Diags`, since they aren't used.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:156-159
+template <typename T>
+FlagToValueNormalizer<T> makeFlagToValueNormalizer(T Value) {
+  return FlagToValueNormalizer<T>{std::move(Value)};
 }
----------------
Please declare this `static`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83694