[PATCH] libc++: Add option to disable access to the global filesystem namespace

Eric Fiselier eric at efcs.ca
Thu Mar 12 11:15:57 PDT 2015


REPOSITORY
  rL LLVM

================
Comment at: libcxx/trunk/include/__config:731
@@ +730,3 @@
+// used, as they attempt to access the global filesystem namespace.
+#ifdef __CloudABI__
+#define _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE
----------------
jroelofs wrote:
> EricWF wrote:
> > jroelofs wrote:
> > > EricWF wrote:
> > > > Where do we get the definition for `__CloudABI__` from?
> > > Compiler provides it, just like `__APPLE__` and all the others.
> > Where can I find more information about when Clang defines this macro?
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150302/264362.html
> 
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20150309/125077.html
Thanks. Since `_LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE` is set in the `__config` file I don't really see why we need to manually define it in CMake and LIT unless we want to be able to test these configurations on platforms which are not `__CloudABI__` (ex FreeBSD which was mentioned).

Can someone explain why we need this?

================
Comment at: libcxx/trunk/test/libcxx/test/format.py:63
@@ -62,1 +62,3 @@
 
+        if test.config.unsupported:
+            return (lit.Test.UNSUPPORTED,
----------------
jroelofs wrote:
> EricWF wrote:
> > jroelofs wrote:
> > > EricWF wrote:
> > > > I don't think we should be using this. Why not put the metadata into the tests?
> > > This is how the tests in llvm/test/CodeGen work. I think it cleans things up **significantly**. I view it as a mistake that I did the libcpp-has-no-threads UNSUPPORTEDs this way.... it's a bit painful to maintain them.
> > My concern is that we accidentally don't run tests because they were mistakenly put in a directory with a lit.site.cfg that marks all the tests as unsupported.
> > 
> > I also think that lit.TestRunner.parseIntegratedTestScript may already handle this but I'll have to double check.
> > My concern is that we accidentally don't run tests because they were mistakenly put in a directory with a lit.site.cfg that marks all the tests as unsupported.
> 
> `--show-unsupported` takes care of that, no?
> 
> > I also think that lit.TestRunner.parseIntegratedTestScript may already handle this but I'll have to double check.
> 
> Apparently not, or there's some flag that I missed that makes it happen... I debugged it a bit this morning before suggesting these lines to @ed
`--show-unsupported` should *help* but you still need to be watching it carefully and understand what tests are actually unsupported.

At least with requiring the metadata in the test I can check the commit that put that line in there. If a test is added to a directory that has a `lit.site.cfg` it's a little harder to figure out.

I don't really mind the extra maintenance cost because it make things more explicit and clear to readers of the test.

Anyway I don't strongly object to this change so I'll drop the subject.

http://reviews.llvm.org/D8194

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the cfe-commits mailing list