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

Prevent Chrome from crashing on large function lists. #858

Merged
merged 8 commits into from
Jan 25, 2024

Conversation

macetw
Copy link
Contributor

@macetw macetw commented Dec 1, 2023

Designed to fix this issue: #857

Designed to base on top of this PR: #840

Work remaining: update reference HTML files. (which was also needed on commits from PR 840)

@macetw macetw requested a review from Spacetown December 1, 2023 19:50
@Spacetown
Copy link
Member

Spacetown commented Dec 1, 2023

@macetw I'll first merge my PR and then rebase your PR to update the reference.

@Spacetown Spacetown force-pushed the break_function_list_when_huge branch from 408babd to 6810733 Compare December 1, 2023 23:12
Copy link
Member

@Spacetown Spacetown left a comment

Choose a reason for hiding this comment

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

LGFM. Please can you check the change from {% if loop.first or loop.index % 10000 == 1 %} to {% if loop.index0 % 10000 == 0 %}.

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ca5b70d) 95.39% compared to head (0cb5a9f) 95.39%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #858   +/-   ##
=======================================
  Coverage   95.39%   95.39%           
=======================================
  Files          54       54           
  Lines        4496     4496           
  Branches      879      879           
=======================================
  Hits         4289     4289           
  Misses        126      126           
  Partials       81       81           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@macetw
Copy link
Contributor Author

macetw commented Dec 4, 2023

LGFM. Please can you check the change from {% if loop.first or loop.index % 10000 == 1 %} to {% if loop.index0 % 10000 == 0 %}.

I will test. Honestly, my last test didn't actually test it in Chrome -- I tested with very small sample sizes and small thresholds (3, rather than 10000). But truly, I need to test in Chrome. Today, I realized how to do this. (I was waiting for a colleague for vacation, but I found I can do this without him). It will take a little time to do this test to get all the data.

@Spacetown
Copy link
Member

@macetw If this PR is merged I want to create a release.

@Spacetown
Copy link
Member

@macetw Have you found time to test this?

@Spacetown Spacetown merged commit 034d3e9 into gcovr:main Jan 25, 2024
27 checks passed
@Spacetown
Copy link
Member

Thanks for working on this.

@Spacetown Spacetown removed this from the Upcoming release milestone Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants