[PATCH] D31739: Add markup for libc++ dylib availability

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini marked 5 inline comments as done.
mehdi_amini added inline comments.


================
Comment at: utils/libcxx/test/config.py:289
+    def configure_availability(self):
+        # FIXME doc
+        self.with_availability = self.get_lit_bool('with_availability', False)
----------------
jroelofs wrote:
> Can you expand on what the FIXME here wants? Is there something that AvailabilityMarkup.rst doesn't cover?
I wrote the FIXME before writing the doc. I added a link to the doc instead.


================
Comment at: utils/libcxx/test/config.py:316
+        if self.with_availability:
+            self.use_clang_verify = False
+            return
----------------
jroelofs wrote:
> Why does the availability stuff clash with -verify?
The syntax verification phase isn't expecting too see compile errors from the availability.


================
Comment at: utils/libcxx/test/config.py:358
 
+    def add_deployement_feature(self, feature):
+        (arch, name, version) = self.config.deployment
----------------
arphaman wrote:
> Typo: `add_deployment_feature`
French style :)


================
Comment at: utils/libcxx/test/config.py:363
+        self.config.available_features.add('%s=%s%s' % (feature, name, version))
+        self.config.available_features.add('%s=%s%s' % (feature, name, version))
+
----------------
jroelofs wrote:
> This line, and the one above it are the same. Is that intentional?
No, just bad copy/paste :)


================
Comment at: utils/libcxx/test/config.py:387
+                self.config.available_features.add(
+                    'with_system_cxx_lib=%s' % component)
 
----------------
jroelofs wrote:
> Is it worth filtering out `none` and `unknown`, as they're often repeated, and you can't tell which part of the triple they came from?
> 
> Consider: `arm-none-linux-gnueabi` vs `arm-none-none-eabi`.
> 
> Or would it be better to include some marker in the features to say which part of the triple it came from, eg:
> 
> ```
>   - with_system_cxx_lib=arch:arm
>   - with_system_cxx_lib=vendor:none
>   - with_system_cxx_lib=os:linux
>   - with_system_cxx_lib=sys:gnueabi
> ```
I like this idea! 
It seems like orthogonal to the availability introduced here, so it'd belong to a separate patch I think.


https://reviews.llvm.org/D31739