[Lldb-commits] [PATCH] D133639: [lldb] Fix detection of existing libcxx

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.


================
Comment at: lldb/test/API/lit.cfg.py:181-184
+if is_configured_and_true('has_libcxx') and platform.system() != 'Windows':
   if is_configured('llvm_include_dir') and is_configured('llvm_libs_dir'):
     dotest_cmd += ['--libcxx-include-dir', os.path.join(config.llvm_include_dir, 'c++', 'v1')]
     dotest_cmd += ['--libcxx-library-dir', config.llvm_libs_dir]
----------------
JDevlieghere wrote:
> Are there other places where this helper would be useful? If not I think we can check the value directly. With the helper we lose a little bit of signal (if it returns false, was it not configured or set to false?). I don't think it's a big deal, but those two things combined make me favor inlining the check. 
I agree that inlining is better here, but note that `config.has_libcxx` doesn't work, this is still going to return `True`: `config.has_libcxx` is a string, not a boolean, so anything `!= None` -- in particular '0' -- will evaluate to true.

There are two ways to inline this: 

1.`is_configured('has_libcxx') and config.has_libcxx == '1'`
2.`is_configured('has_libcxx') == '1'`

Actually, since `has_libcxx` comes from a canonicalized variable (in the sense described in the commit message), we could also just do:

3.`config.has_libcxx == '1'`


What do you think?

Related: in a separate patch I think we should change the implementation of `is_configured` to compare against None, so that it returns a boolean.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133639