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

Implement PROBLEMATIC_CONSTS generalization #115253

Merged
merged 25 commits into from Apr 19, 2024
Merged

Conversation

donno2048
Copy link
Contributor

@donno2048 donno2048 commented Aug 26, 2023

You forgot that A≈4, B≈8, and E≈3 and some more constants.

The new PROBLEMATIC_CONSTS was generated using this code:

from functools import reduce
def generate_problems(consts: list, letter_digit: dict):
    for const in consts:
        problem = reduce(lambda string, rep: string.replace(*reversed(rep)), ['%X' % const, *letter_digit.items()])
        indexes = [index for index, c in enumerate(problem) if c in letter_digit.keys()]
        for i in range(1 << len(indexes)):
            yield int(''.join(letter_digit[c] if index in indexes and (i >> indexes.index(index)) & 1 else c for index, c in enumerate(problem)), 0x10)


problems = generate_problems(
    [
        # Old PROBLEMATIC_CONSTS:
        184594741, 2880289470, 2881141438, 2965027518, 2976579765, 3203381950, 3405691582, 3405697037,
        3735927486, 3735932941, 4027431614, 4276992702,
        # More of my own:
        195934910, 252707358, 762133, 179681982, 173390526
    ],
    {
    'A': '4',
    'B': '8',
    'E': '3',
    }
)

# print(list(problems)) # won't use that to print formatted

from itertools import islice
while len(cur_problems := list(islice(problems, 8))):
    print('    ', end='')
    print(*cur_problems, sep=', ', end='')
    print(',')

@rustbot
Copy link
Collaborator

rustbot commented Aug 26, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 26, 2023
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

Would it be possible to keep just the short list of root problematic consts, and then put your generate_problems logic into tidy, instead of expanding it all out like this?

That would be much more easily auditable.

@donno2048
Copy link
Contributor Author

@dtolnay I thought because the list is static it'd be better to make it hardcoded, wouldn't it?

@dtolnay
Copy link
Member

dtolnay commented Sep 17, 2023

I don't know a reason that it would need to be static.

@rust-log-analyzer

This comment has been minimized.

src/tools/tidy/src/style.rs Outdated Show resolved Hide resolved
src/tools/tidy/src/style.rs Outdated Show resolved Hide resolved
src/tools/tidy/src/style.rs Outdated Show resolved Hide resolved
@dtolnay dtolnay assigned dtolnay and unassigned joshtriplett Sep 17, 2023
src/tools/tidy/src/style.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@dtolnay dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 17, 2023
@rust-log-analyzer

This comment has been minimized.

@dtolnay dtolnay assigned joshtriplett and unassigned dtolnay Sep 19, 2023
@donno2048

This comment was marked as off-topic.

@donno2048
Copy link
Contributor Author

But also correct me if I'm wrong, but without using the generate_problems function the (PROBLEMATIC_CONSTS.iter().map(u32::to_string)) won't work as expected

@donno2048 donno2048 changed the title Add more PROBLEMATIC_CONSTS Implement PROBLEMATIC_CONSTS generalization Sep 27, 2023
@dtolnay
Copy link
Member

dtolnay commented Sep 27, 2023

Yes, good point. The regex approach (.replace("a", "[aA4]") etc.) results in not finding capitalization variations for values that are problematic in hex but written as decimal in the source code.

I don't know the importance of looking for the decimal converted values.

I have assigned back to @joshtriplett who created this lint originally.

@dtolnay dtolnay removed their request for review September 27, 2023 23:49
@donno2048
Copy link
Contributor Author

I think if we are ignoring some cases of (PROBLEMATIC_CONSTS.iter().map(u32::to_string)) we should remove it completely

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@donno2048
Copy link
Contributor Author

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 18, 2024
Copy link
Member

@albertlarsan68 albertlarsan68 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR!

@albertlarsan68
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 19, 2024

📌 Commit e2ab540 has been approved by albertlarsan68

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 19, 2024
@bors
Copy link
Contributor

bors commented Apr 19, 2024

⌛ Testing commit e2ab540 with merge 43a0686...

@bors
Copy link
Contributor

bors commented Apr 19, 2024

☀️ Test successful - checks-actions
Approved by: albertlarsan68
Pushing 43a0686 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 19, 2024
@bors bors merged commit 43a0686 into rust-lang:master Apr 19, 2024
13 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 19, 2024
@donno2048 donno2048 deleted the patch-1 branch April 19, 2024 12:05
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (43a0686): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.2% [0.2%, 0.2%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [0.2%, 0.2%] 2

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 671.432s -> 673.234s (0.27%)
Artifact size: 315.27 MiB -> 315.21 MiB (-0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants