vgvassilev / clad

clad -- automatic differentiation for C/C++
GNU Lesser General Public License v3.0
266 stars 113 forks source link

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

Closed vgvassilev closed 3 weeks ago

vgvassilev commented 3 weeks ago

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

github-actions[bot] commented 3 weeks ago

clang-tidy review says "All clean, LGTM! :+1:"

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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](https://app.codecov.io/gh/vgvassilev/clad/pull/930/graphs/tree.svg?width=650&height=150&src=pr&token=9f6Q4em8hE&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev)](https://app.codecov.io/gh/vgvassilev/clad/pull/930?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev) ```diff @@ 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](https://app.codecov.io/gh/vgvassilev/clad/pull/930?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev) | Coverage Δ | | |---|---|---| | [lib/Differentiator/VisitorBase.cpp](https://app.codecov.io/gh/vgvassilev/clad/pull/930?src=pr&el=tree&filepath=lib%2FDifferentiator%2FVisitorBase.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev#diff-bGliL0RpZmZlcmVudGlhdG9yL1Zpc2l0b3JCYXNlLmNwcA==) | `96.65% <100.00%> (ø)` | | ... and [6 files with indirect coverage changes](https://app.codecov.io/gh/vgvassilev/clad/pull/930/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev) | [Files](https://app.codecov.io/gh/vgvassilev/clad/pull/930?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev) | Coverage Δ | | |---|---|---| | [lib/Differentiator/VisitorBase.cpp](https://app.codecov.io/gh/vgvassilev/clad/pull/930?src=pr&el=tree&filepath=lib%2FDifferentiator%2FVisitorBase.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev#diff-bGliL0RpZmZlcmVudGlhdG9yL1Zpc2l0b3JCYXNlLmNwcA==) | `96.65% <100.00%> (ø)` | | ... and [6 files with indirect coverage changes](https://app.codecov.io/gh/vgvassilev/clad/pull/930/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Vassil+Vassilev)
vgvassilev commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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.

github-actions[bot] commented 3 weeks ago

clang-tidy review says "All clean, LGTM! :+1:"

vgvassilev commented 3 weeks ago

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