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

Fix an assertion when building the clad benchmarks with clang18 on osx. #930

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

vgvassilev
Copy link
Owner

Unfortunately, it'd be difficult to come up with a test...

@vgvassilev vgvassilev requested a review from vaithak June 10, 2024 08:16
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.72%. Comparing base (b38d8cf) to head (6bfae40).
Report is 8 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #930      +/-   ##
==========================================
- Coverage   94.08%   93.72%   -0.36%     
==========================================
  Files          53       54       +1     
  Lines        7762     7782      +20     
==========================================
- Hits         7303     7294       -9     
- Misses        459      488      +29     
Files Coverage Δ
lib/Differentiator/VisitorBase.cpp 96.65% <100.00%> (ø)

... and 6 files with indirect coverage changes

Files Coverage Δ
lib/Differentiator/VisitorBase.cpp 96.65% <100.00%> (ø)

... and 6 files with indirect coverage changes

Copy link
Collaborator

@vaithak vaithak left a comment

Choose a reason for hiding this comment

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

Looks good.
But, given that we will improve the error or diagnostic messages in the future, maybe we can use E->getBeginLoc and E->getEndLoc instead of creating fake ones.

@vgvassilev
Copy link
Owner Author

Looks good. But, given that we will improve the error or diagnostic messages in the future, maybe we can use E->getBeginLoc and E->getEndLoc instead of creating fake ones.

Until now we explicitly tried to use the fakeLoc approach to consistently annotate where we need to handle source locations in a non-crashing way. We could but that would point to the original function. Is that what you want here?

@vaithak
Copy link
Collaborator

vaithak commented Jun 10, 2024

Until now we explicitly tried to use the fakeLoc approach to consistently annotate where we need to handle source locations in a non-crashing way. We could but that would point to the original function. Is that what you want here?

Although it would point to the original function, it will be really helpful for creating minimal reproducers from big functions, as these location informations in the primal function will point to the line or location of the troublesome expression.

@vgvassilev
Copy link
Owner Author

Until now we explicitly tried to use the fakeLoc approach to consistently annotate where we need to handle source locations in a non-crashing way. We could but that would point to the original function. Is that what you want here?

Although it would point to the original function, it will be really helpful for creating minimal reproducers from big functions, as these location informations in the primal function will point to the line or location of the troublesome expression.

Ok. Let me change that. However, this comment is questioning our currently strategy here, right? If you agree we should open an issue where we suggest replacing all of the fake locations with ones pointing to the original function.

@vaithak
Copy link
Collaborator

vaithak commented Jun 10, 2024

Ok. Let me change that. However, this comment is questioning our currently strategy here, right? If you agree we should open an issue where we suggest replacing all of the fake locations with ones pointing to the original function.

I am in full support for the new strategy of pointing to original function locations, so that it provides ease of debugging.

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@vgvassilev
Copy link
Owner Author

I do not think the codecov complaint is relevant for this PR. Let's move forward.

@vgvassilev vgvassilev merged commit 6efcd08 into master Jun 11, 2024
88 of 89 checks passed
@vgvassilev vgvassilev deleted the fake-locs branch June 11, 2024 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants