lenary added a comment. I have some nits about explicit comments for arguments and default argument values for backwards compatibility. Other than that, it looks like a nice code cleanup.
================ Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6071 // Add the target features - getTargetFeatures(getToolChain(), Triple, Args, CmdArgs, true); + getTargetFeatures(getToolChain(), Triple, Args, CmdArgs, true, false); ---------------- This needs `/*ForAS=*/true, /*ForLTOPlugin=*/false)` so it's clear what the booleans are for. Same for other calls to `getTargetFeatures`. ================ Comment at: clang/lib/Driver/ToolChains/CommonArgs.h:134 + llvm::opt::ArgStringList &CmdArgs, bool ForAS, + bool ForLTOPlugin); + ---------------- The final argument here should probably default to `false`, so that out-of-tree backends do not break immediately when this patch lands. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67409/new/ https://reviews.llvm.org/D67409 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
