Skip to content

Conversation

@jamesr66a
Copy link
Collaborator

@jamesr66a jamesr66a commented Oct 30, 2019

Stack from ghstack:

The previous regex caused a std::regex_error under clang8 complaining about error_brack, which is strange because the square brackets are balanced. Seems like a stdlib bug to me. So to workaround this, I've switched to the older regex with a non-greedy match in the inner atom

Differential Revision: D18232654

@jamesr66a jamesr66a requested a review from apaszke as a code owner October 30, 2019 20:51
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Oct 30, 2019
@jamesr66a jamesr66a requested a review from soumith October 30, 2019 20:53
Copy link
Contributor

@soumith soumith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @ggoossen it reverts your change because of Clang8 issue

@jamesr66a
Copy link
Collaborator Author

@soumith not really a revert but just an alternate implementation to fix the same issue, notice the new ? in the regex

@facebook-github-bot
Copy link
Contributor

@jamesr66a merged this pull request in 15deee2.

@soumith
Copy link
Contributor

soumith commented Oct 31, 2019

ah okay, didn't notice

@facebook-github-bot facebook-github-bot deleted the gh/jamesr66a/135/head branch November 3, 2019 15:15
@ggoossen
Copy link
Contributor

ggoossen commented Nov 4, 2019

@jamesr66a Adding the ? won't fix it, it still matches {} although with empty capture, you'll have to filter these out, before deciding there are no matches.

@jamesr66a
Copy link
Collaborator Author

@ggoossen in that case the inner atom should have + instead of * i think: https://regex101.com/r/hnQBmE/1

ggoossen added a commit that referenced this pull request Nov 5, 2019
…attempt

Previous attempt in #28616 didn't work in clang8 and got reverted in #28916
This one escapes the } inside the brackets, maybe that works

Differential Revision: [D18324286](https://our.internmc.facebook.com/intern/diff/D18324286/)

[ghstack-poisoned]
ggoossen added a commit that referenced this pull request Nov 5, 2019
…attempt

Previous attempt in #28616 didn't work in clang8 and got reverted in #28916
This one escapes the } inside the brackets, maybe that works

Differential Revision: [D18324286](https://our.internmc.facebook.com/intern/diff/D18324286/)

ghstack-source-id: 93270628
Pull Request resolved: #29193
ggoossen added a commit that referenced this pull request Nov 5, 2019
…ns, second attempt"

Previous attempt in #28616 didn't work in clang8 and got reverted in #28916
This one escapes the } inside the brackets, maybe that works

Differential Revision: [D18324286](https://our.internmc.facebook.com/intern/diff/D18324286/)

[ghstack-poisoned]
ggoossen added a commit that referenced this pull request Nov 5, 2019
…attempt

Pull Request resolved: #29193

Previous attempt in #28616 didn't work in clang8 and got reverted in #28916
This one escapes the } inside the brackets, maybe that works
ghstack-source-id: 93283014

Differential Revision: [D18324286](https://our.internmc.facebook.com/intern/diff/D18324286/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants