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

Fri Feb 8 16:04:17 PST 2019

aheejin marked an inline comment as done.
aheejin 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)
----------------
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.

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57874/new/

https://reviews.llvm.org/D57874