[PATCH] D57874: [WebAssembly] Make thread-related options consistent

Fri Feb 8 16:14:07 PST 2019

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