jansvoboda11 added a comment. I've added tests for OptParserEmitter. Let me know if you had something more detailed in mind.
================ 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">; ---------------- dexonsmith wrote: > jansvoboda11 wrote: > > dexonsmith wrote: > > > 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. > > I agree that making all `OptInFFlag` and `OptOutFFlag` driver flags as well > > as `-cc1` flags by default is not great. How would we go about deciding > > which options are okay to demote to `-cc1`-only? Perhaps those not present > > in `ClangCommandLineReference.rst` and driver invocations in tests? > > How would we go about deciding which options are okay to demote to > > `-cc1-only`? > > The key is not to add (or remove) driver options unintentionally. Driver > options are `clang`'s public interface, and once an option shows up there > we're supposed to support it "forever". We shouldn't be > accidentally/incidentally growing that surface area in order to simplify > parsing/generating `-cc1` command-lines. > > I based my comment on @dang's reason for not using `OptInFFLag`, which I > agree with: > > 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. > Agreed. ================ Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:468-469 + // Restore the definition order of marshalling options. + array_pod_sort(OptsWithMarshalling.begin(), OptsWithMarshalling.end(), + CmpMarshallingOpts); + ---------------- dexonsmith wrote: > jansvoboda11 wrote: > > dexonsmith wrote: > > > 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, ...)); > > > ``` > > 1. The options get out of order during parsing. The `RecordKeeper` stores > > records in `std::map<std::string, std::unique_ptr<Record>, std::less<>>` > > that maintains lexicographical order. > > > > 2. Later, they are reordered in this function before prefix groups are > > generated: `array_pod_sort(Opts.begin(), Opts.end(), > > CompareOptionRecords);`. > > > > 3. Before we generate the marshalling code, we need to restore the > > definition order so that we don't use a `LangOpts` or `CodeGenOpts` field > > (from `DefaultAnyOf`) before it was initialized. > > > > I've added more detailed explanation to the comment. > > > > I used `array_pod_sort` to be consistent with what's already used here in > > `OptParserEmitter.cpp`. I will switch to `llvm::sort` to be more concise if > > we don't mind the potential code bloat described here > > <https://llvm.org/doxygen/namespacellvm.html#add1eb5637dd671428b6f138ed3db6428>. > Thanks for the explanation about the ordering, this makes sense. > > Regarding `array_pod_sort`, I was referring to how `llvm::sort` calls > `array_pod_sort` when it can... but I hadn't noticed before that it can't do > this for a custom comparator. You should stick with `array_pod_sort` > (although maybe as a follow-up I'll look into whether we can detect if the > custom comparator to `llvm::sort` is stateless and defer to `array_pod_sort` > in that case as well...) No problem. I've switched back to `array_pod_sort`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82756/new/ https://reviews.llvm.org/D82756 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
