dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.
LGTM if you undo the change to lose support for AlwaysEmit (happy to consider
in a separate patch if it’s the right thing to do though).
================
Comment at: clang/include/clang/Driver/Options.td:371
+ // TODO: Assert that the flags have different value.
+ // TODO: Assert that only one of the flags can be implied.
+
----------------
jansvoboda11 wrote:
> Does TableGen support some kind of assertion mechanism?
Not that I’m aware of. @Bigcheese?
================
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,
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92775/new/
https://reviews.llvm.org/D92775
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
