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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits