sbc100 added inline comments.
================ Comment at: lib/Driver/ToolChains/WebAssembly.cpp:66 + if (Args.hasFlag(clang::driver::options::OPT_pthread, + clang::driver::options::OPT_no_pthread), + false) ---------------- aheejin wrote: > tlively wrote: > > sbc100 wrote: > > > aheejin wrote: > > > > This code is not strictly related, but `hasFlag` is better than > > > > `hasArg` when there are both positive and negative versions of an > > > > option exist. > > > Hmm.. there are currently no other references to OPT_no_pthread in the > > > whole codebase. Maybe better to simply remove the option? > > > > > > I wouldn't want to commit this as that first use of the option as it > > > might make it hard to remove :) > > I think commands generally come in pairs to make it possible to override > > previous settings by appending args to command lines. Counting uses of > > OPT_no_pthread without including uses of OP_pthread doesn't make much sense. > Most true/false or enable/disable options exist in pairs. `-no-pthread` is > defined [[ > https://github.com/llvm/llvm-project/blob/91970564191bfc40ea9f2c8d32cc1fb6c314515c/clang/include/clang/Driver/Options.td#L2508 > | here ]]. So this `ArgList::hasFlag` function checks the existence of both > the positive option and the negative option at the same time, and if neither > exists, takes the default value, which is the third argument. So yes, as > @tlively said, it's just a safety measure. Before it only checked the > existence of `-pthread` and didn't care if `-no-pthread` existed or not. There no current usage of OPT_no_pthread anywhere in clang. For this reason I think this option is ripe for removal completely. Therefore I don't think its a good idea to introduce a usage here, as it could make the removal more difficult. Especially as this part of the patch is kind of unrelated.. its kind of an authogonal cleanup .. which I think makes things worse. So I vote to revert this line :) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57874/new/ https://reviews.llvm.org/D57874 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
