[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

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

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

I like how this is coming together. I have a few comments inline.

Also, I wonder if there should be a test for the new OptParser behaviour in `llvm/unittests/Option/`.



================
Comment at: clang/include/clang/Driver/Options.td:1176
+defm reciprocal_math : OptInFFlag< "reciprocal-math", "Allow division operations to be reassociated", "", "", [], "LangOpts->AllowRecip">;
+def fapprox_func : Flag<["-"], "fapprox-func">, Group<f_Group>, Flags<[CC1Option, NoDriverOption]>,
+  MarshallingInfoFlag<"LangOpts->ApproxFunc", "false">;
----------------
dang wrote:
> Anastasia wrote:
> > could this also be OptInFFlag?
> The aim was to keep the driver semantics the same as before and this was not something you could control with the driver, so I left it as just a CC1 flag. However if it makes sense to be able to control this from the driver then we can definitely make this `OptInFFLag`.
I think adding a driver flag (if that's the right thing to do) should be done separately in a follow-up commit.

Also for a separate commit: it would be a great improvement if you could have OptIn / OptOut flags that were `-cc1`-only (maybe `CC1OptInFFlag`).
- Both `-fX` and `-fno-X` would be parsed by `-cc1` (and cancel each other out).
- Only the non-default one would be generated when serializing to `-cc1` from `CompilerInvocation` (for `OptIn`, we'd never generate `-fno-X`).
- Neither would be recognized by the driver.

I suggest we might want that for most `-cc11` flags. This would make it easier to poke through the driver with `-Xclang` to override `-cc1` options the driver adds. Not something we want for end-users, but useful for debugging the compiler itself. Currently the workflow is to run the driver with `-###`, copy/paste, search for and remove the option you want to skip, and finally run the `-cc1` command...

The reason I bring it up is that it's possible people will start using `OptInFFLag` just in order to get this behaviour, not because they intend to add a driver flag.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3707
 #undef OPTION_WITH_MARSHALLING_FLAG
+
   return true;
----------------
I don't have an opinion about whether there should be a newline here, but please make unrelated whitespace changes like this in a separate commit (before/after).


================
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:460-464
+    if (AID < BID)
+      return -1;
+    if (AID > BID)
+      return 1;
+    return 0;
----------------
I think `array_pod_sort` will use this like a `bool`, similar to `std::sort`, in which case you I think you want:
```
  return (*A)->getID() < (*B)->getID();
```


================
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:468-469
+  // Restore the definition order of marshalling options.
+  array_pod_sort(OptsWithMarshalling.begin(), OptsWithMarshalling.end(),
+                 CmpMarshallingOpts);
+
----------------
I'm curious if this is necessary. If so, how do the options get out-of-order?

Also, a cleaner way to call `array_pod_sort` would be:
```
llvm::sort(OptsWithMarshalling, CmpMarshallingOpts);
```
and I would be tempted to define the lambda inline in the call to `llvm::sort`.

If it's not necessary, I suggest replacing with an assertion:
```
assert(llvm::is_sorted(OptsWithMarshalling, ...));
```


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

https://reviews.llvm.org/D82756