Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[third-party] Mark inst2vec regexes as regex literals. #594

Merged

Conversation

ChrisCummins
Copy link
Contributor

It is now deprecated to use regex escape characters in string literals.

This adds the "r" string prefix to the regex literals in compiler_gym/third_party/inst2vec/inst2vec_preprocess.py.

It is now deprecated to use regex escape characters in string literals..
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 3, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2022

Codecov Report

Merging #594 (656efaa) into development (0394e60) will decrease coverage by 8.19%.
The diff coverage is 75.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #594      +/-   ##
===============================================
- Coverage        88.17%   79.97%   -8.20%     
===============================================
  Files              114      114              
  Lines             6708     6708              
===============================================
- Hits              5915     5365     -550     
- Misses             793     1343     +550     
Impacted Files Coverage Δ
compiler_gym/third_party/inst2vec/rgx_utils.py 86.53% <ø> (ø)
...er_gym/third_party/inst2vec/inst2vec_preprocess.py 10.72% <75.00%> (-63.22%) ⬇️
...loop_tool/service/loop_tool_compilation_session.py 14.96% <0.00%> (-74.83%) ⬇️
compiler_gym/envs/llvm/compute_observation.py 31.81% <0.00%> (-59.10%) ⬇️
compiler_gym/envs/llvm/datasets/clgen.py 43.28% <0.00%> (-46.27%) ⬇️
compiler_gym/envs/llvm/llvm_benchmark.py 51.09% <0.00%> (-42.34%) ⬇️
compiler_gym/third_party/inst2vec/__init__.py 56.66% <0.00%> (-36.67%) ⬇️
compiler_gym/envs/llvm/datasets/llvm_stress.py 58.33% <0.00%> (-33.34%) ⬇️
compiler_gym/envs/llvm/datasets/chstone.py 51.21% <0.00%> (-31.71%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0394e60...656efaa. Read the comment docs.

Copy link
Contributor

@hughleat hughleat left a comment

Choose a reason for hiding this comment

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

LGTM

array_of_array_type = "\[\d+ x " + "\[\d+ x " + rgx.first_class_type + "\]" + "\]"
vector_type = r"<\d+ x " + rgx.first_class_type + r">"
array_type = r"\[\d+ x " + rgx.first_class_type + r"\]"
array_of_array_type = r"\[\d+ x " + r"\[\d+ x " + rgx.first_class_type + r"\]" + r"\]"
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider rf-strings?

array_of_array_type = rf"\[\d+ x \[\d+ x {rgx.first_class_type}\]\]"

else:
s += possibilities[0]
for i in range(len(possibilities) - 1):
if len(to_add) > 0:
s += "|" + possibilities[i + 1] + to_add + " "
s += r"|" + possibilities[i + 1] + to_add + r" "
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it's wrong, but... why? I can see it when you have backslashes, but there are lots of changes here with no backslashes. Is it sensible to use additional abstractions which aren't necessary?
IMO, only use r-strings where necessary. Use f-strings or rf-strings a lot more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Its an automatic refactoring of third party code. Rewriting the regexes to rf-strings would be nice, but I don't know how to do it automatically

@ChrisCummins ChrisCummins merged commit f8a5117 into facebookresearch:development Mar 3, 2022
@ChrisCummins ChrisCummins deleted the fix/inst2vec-regex branch March 3, 2022 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants