This is an archive of the discontinued LLVM Phabricator instance.

A number of improvements to decorator conditionals
ClosedPublic

Authored by zturner on Feb 5 2016, 4:37 PM.

Details

Summary

The goal here is to try to improve consistency across the variety of decorators we support. This patch makes the following changes:

  • Change the not_in function to be a class object called no_match. This indicates that it can be used for more than just checking list membership, and that instead it negates whatever condition is used to normally match.
  • Change the name of _check_list_or_lambda to match_decorator_property. This indicates that it could be more than a list or a lambda.
  • Always use a regex match when matching strings in match_decorator_property. This allows any string on any decoratory property to automatically support regex matching.
  • Also support the use of compiled regexes on decorator properties.
  • Fix a bug in the compiler check used by _decorateTest. The two arguments were reversed, so that check was not matching when it should have matched.
  • Change one test that uses skipUnlessArch to use skipIf. Now that skipIf supports regex matching on the architecture property, this works, and as a followup skipUnlessArch will be removed entirely.

Note that you won't be able to apply this patch unless you apply it on top of my previous review D16936.

Diff Detail

Event Timeline

zturner updated this revision to Diff 47063.Feb 5 2016, 4:37 PM
zturner retitled this revision from to A number of improvements to decorator conditionals.
zturner updated this object.
zturner added reviewers: tfiala, labath, tberghammer.
zturner added a subscriber: lldb-commits.
zturner updated this revision to Diff 47068.Feb 5 2016, 5:26 PM

Minor change that allows any iterable to be used (not just lists), and also recurses through the _match_decorator_property function for each item in the iterable, instead of just doing a simple list membership. Combined with the regex change and the no_match change from function to object, this allows complex specifications such as archs=['mips.*', 'i[0-9]86', no_match('x86_x64)'] (not that that particular condition makes sense, but might as well support it).

tfiala edited edge metadata.Feb 5 2016, 7:10 PM

I'll have a look at this after I get through D16936.

Concept sounds great.

labath accepted this revision.Feb 8 2016, 3:08 AM
labath edited edge metadata.

Looks good, after applying one fix.

packages/Python/lldbsuite/test/decorators.py
150

The actual value should be simply lldb.remote_platform is not None. When remote_platform is None, we are local, when it is not None, we are remote.

Btw, since this flag can only have three values (None, True, False), the _match_decorator property dance is not really necessary. Although, I guess it won't hurt either...

This revision is now accepted and ready to land.Feb 8 2016, 3:08 AM
tfiala accepted this revision.Feb 8 2016, 10:04 AM
tfiala edited edge metadata.

LGTM.

Regarding the section Pavel was looking at, on the Apple side we have a simulator target that is not technically remote, but isn't really local either. I'll have to look at that if/when we actually want to mark a simulator test as failed.

This revision was automatically updated to reflect the committed changes.