[libcxx-commits] [PATCH] D59152: [libc++] Build <filesystem> support as part of the dylib
Eric Fiselier via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Mar 19 08:42:48 PDT 2019
EricWF added inline comments.
Comment at: libcxx/utils/libcxx/test/target_info.py:244
- flags += ['-lpthread']
- if not shared_libcxx or enable_fs:
- flags += ['-lrt']
+ flags += ['-lpthread', '-lrt']
flags += ['-lc']
> EricWF wrote:
> > This change seems dangerously incorrect.
> > When testing the dylib, we don't want to link any libraries not linked by clang by default. Please keep this as it `-lrt` as it was.
> So what I did was only substitute `enable_fs` for `True` because in the case of libc++ this will always be true, and then I was able to remove the `if` statement altogether. SO we're already always linking against `-lrt` in all tests when `enable_filesystem=True` today. I think the _actual_ problem is the `FIXME` above, where we should be more granular in linking `-lrt` only when for `<filesystem>` tests. Does that make sense? If you agree with that, I'm happy to work on fixing that `FIXME` as a follow-up patch.
The check was essentially checking if we had any static libraries. It checked for filesystem because filesystem was always static. When it's in the dylib the explicit `-lrt` shouldn't be needed.
Let's address the FIXME and only link `-lrt` when `not shared_libcxx`.
CHANGES SINCE LAST ACTION
More information about the libcxx-commits