[libcxx-commits] [PATCH] D88027: [libcxx] Add platforms and targets to available features.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.
Herald added a subscriber: dexonsmith.


================
Comment at: libcxx/utils/libcxx/test/config.py:270-284
+        if platform.system() == 'Darwin':
+            self.config.available_features.add('system-linker-mach-o')
+            self.config.available_features.add('system-darwin')
+        elif platform.system() == 'Windows':
+            self.config.available_features.add('system-windows')
+        elif platform.system() == 'Linux':
+            self.config.available_features.add('system-linux')
----------------
None of these features are used anywhere in the tests as far as I can find. Why add them?


================
Comment at: libcxx/utils/libcxx/test/config.py:289
+        target_triple = getattr(self.config, 'target_triple', None)
+        if host_triple and host_triple == target_triple:
+            self.config.available_features.add('native')
----------------
Not used in the tests, I would prefer we don't add features just for the sake of having them.


================
Comment at: libcxx/utils/libcxx/test/config.py:292-304
+        if target_triple:
+            if re.match(r'^x86_64.*-apple', target_triple):
+                self.config.available_features.add('x86_64-apple')
+            if re.match(r'^x86_64.*-linux', target_triple):
+                self.config.available_features.add('x86_64-linux')
+            if re.match(r'^i.86.*', target_triple):
+                self.config.available_features.add('target-x86')
----------------
I think it only makes sense to add those, and the reason is that Lit doesn't allow matching partial triples. We already add the full target triple as a Lit feature, and I think it would be nicer to be able to say something like `// REQUIRES: x86_64-linux-*` instead of having to add these features. But Lit doesn't do that, so let's add them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88027