[libcxx-commits] [PATCH] D59152: [libc++] Build <filesystem> support as part of the dylib
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Mar 11 08:51:08 PDT 2019
ldionne marked 2 inline comments as done.
ldionne added a comment.
In D59152#1423668 <https://reviews.llvm.org/D59152#1423668>, @EricWF wrote:
> This LGTM other than the inline nits.
> Let me know if you want me to produce the linux ABI list changes.
Yes, that would be nice.
> PS. It would be really nice if we found an easier way to generate all the ABI list changes for a given change at once.
Agreed, but even better would be to use an explicit list of symbols to export. This way, we wouldn't need to _test_ for what symbols are exported, and the list of symbols would be naturally maintained alongside the library.
Comment at: libcxx/utils/libcxx/test/config.py:710
- enable_fs = self.get_lit_bool('enable_filesystem', default=False)
- if not enable_fs:
> There are going to be users and platforms that don't supply filesystem and wish to disable the tests.
> How can they do that?
I view this like `<chrono>`. If an implementation doesn't support `<filesystem>` for some reason, I can create a lit feature like `dylib-has-no-filesystem` in the availability patch and then mark the filesystem tests as unsupported when that lit feature is provided. That's the way we handle partial implementations for other sub-libraries, and I think it should be the way we do it for `<filesystem>` too.
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']
> 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.
CHANGES SINCE LAST ACTION
More information about the libcxx-commits