thakis marked an inline comment as done.
thakis added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3564
+ ArgStringList DummyArgs;
+ CollectArgsForIntegratedAssembler(C, Args, DummyArgs, D,
+ TC.useIntegratedAs());
----------------
thakis wrote:
> rnk wrote:
> > I think it would be better to use ClaimAllArgs here. If you look around in
> > this code, there's a fair amount of stuff like this:
> > ```
> > if (KernelOrKext) {
> > // -mkernel and -fapple-kext imply no exceptions, so claim exception
> > related
> > // arguments now to avoid warnings about unused arguments.
> > Args.ClaimAllArgs(options::OPT_fexceptions);
> > Args.ClaimAllArgs(options::OPT_fno_exceptions);
> > Args.ClaimAllArgs(options::OPT_fobjc_exceptions);
> > Args.ClaimAllArgs(options::OPT_fno_objc_exceptions);
> > Args.ClaimAllArgs(options::OPT_fcxx_exceptions);
> > Args.ClaimAllArgs(options::OPT_fno_cxx_exceptions);
> > return;
> > }
> > ```
> > It repeats the knowledge of which flags are passed to the assembler (looks
> > like 5), but it is consistent with what's already done.
> I think that's worse for this use case here: In addition of having to
> duplicate the flags, the flag parsing error output (for integrated as) is
> then again dependent on if we actually run the assembler, which is the same
> kind of inconsistency that this change is fixing.
I uploaded a patch for the approach you suggested to
https://reviews.llvm.org/D65233 . It's ~20 (non-whitespace) lines of code,
while this patch is ~10 lines (it looks like more because phab doesn't
highlight whitespace-only changes well.)
I like this patch more, but let me know which one you'd like me to land.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65108/new/
https://reviews.llvm.org/D65108
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
