[PATCH] D92775: [clang][cli] Add flexible TableGen multiclass for boolean options

Michael Spencer via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 8 08:37:55 PST 2020


Bigcheese added inline comments.


================
Comment at: clang/unittests/Frontend/CompilerInvocationTest.cpp:226
   ASSERT_THAT(GeneratedArgs,
-              Contains(StrEq("-fno-experimental-new-pass-manager")));
+              Not(Contains(StrEq("-fno-experimental-new-pass-manager"))));
   ASSERT_THAT(GeneratedArgs,
----------------
dexonsmith wrote:
> jansvoboda11 wrote:
> > dexonsmith wrote:
> > > Can you clarify why this was dropped? Was it previously emitted due to a limitation in the implementation, or are we no longer supporting options that always get emitted for clarity?
> > This option was the only one using the old infrastructure (`BooleanMarshalledFFlag`).
> > It was set up to always generate the flag, even when it wasn't necessary (this flag sets the keypath to its default value).
> > 
> > I think we should aim to generate only command line arguments that are necessary to preserve the original invocation semantics.
> > I imagine this will be useful when debugging: one won't need to scan hundreds of flags that don't have any effect on CompilerInvocation.
> This is a change in direction; the original thinking was that some options should always be emitted for human readability. I don’t feel too strongly about it, but I think this should be changed / dropped independently of other work if it’s done. I suggest splitting this out; I’d also like to hear @Bigcheese’s thoughts on that change since he did more of the original reviews. 
In general there are some options that should always be kept, such as the triple, but I don't see any reason to always keep `-fno-experimental-new-pass-manager`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92775



More information about the llvm-commits mailing list