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

Add extended SFINAE capabilities for eckit::Translator #78

Merged
merged 7 commits into from
Dec 6, 2023

Conversation

geier1993
Copy link
Contributor

@geier1993 geier1993 commented Jul 24, 2023

I added a predicate IsTranslatable that test for an existing and valid implementation.

Todo so I had to change the default operator to an auto type - otherwise the return type can be deduced without evaluating the overload set properly.

I'm using this in a new multio feature branch with an variant based Metadata class (instead of LocalConfiguration).
The compilation already included mir (develop), metkit and fdb.

Not all of these packages already use C++17. Hence I added some compiler flags.

@FussyDuck
Copy link

FussyDuck commented Jul 24, 2023

CLA assistant check
All committers have signed the CLA.

@geier1993 geier1993 force-pushed the feature/eckit_translate_sfinae branch from e80dbf8 to 0c7c3fd Compare July 25, 2023 12:14
@geier1993 geier1993 force-pushed the feature/eckit_translate_sfinae branch from 0c7c3fd to 4341b14 Compare September 15, 2023 09:55
@geier1993 geier1993 force-pushed the feature/eckit_translate_sfinae branch from 4341b14 to 66f2706 Compare September 15, 2023 09:57
@geier1993 geier1993 requested a review from simondsmart October 10, 2023 07:35
@geier1993
Copy link
Contributor Author

@simondsmart we talked about these changes before. I renamed VariantHelpers to Overloaded.
The other changes should not break existing software. Dynamic behaviour is still the same, just compile time behaviour is different

Copy link
Contributor

@simondsmart simondsmart left a comment

Choose a reason for hiding this comment

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

After review work today, all approved.


#if __cplusplus >= 201703L

// primary template handles types that do not support pre-increment:
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is clearly not correct. We are not doing any incrementing here. Pre- or otherwise...

}
#else
// If conversion is possible
template <typename F, typename std::enable_if<
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this enable_if is needed. Probably more useful to just get the compiler error if it doesn't work?

@simondsmart simondsmart added the approved-for-ci Approved for CI run label Oct 25, 2023
Copy link
Contributor

@simondsmart simondsmart left a comment

Choose a reason for hiding this comment

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

Please fix the mismatched comment that I've already noted, and then merge.

@github-actions github-actions bot removed the approved-for-ci Approved for CI run label Nov 15, 2023
@simondsmart simondsmart added the approved-for-ci Approved for CI run label Nov 15, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (ffdee81) 62.85% compared to head (a997a9e) 62.19%.
Report is 91 commits behind head on develop.

Files Patch % Lines
src/eckit/utils/Translator.h 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #78      +/-   ##
===========================================
- Coverage    62.85%   62.19%   -0.66%     
===========================================
  Files          796      784      -12     
  Lines        45287    44473     -814     
===========================================
- Hits         28463    27661     -802     
+ Misses       16824    16812      -12     

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

@github-actions github-actions bot removed the approved-for-ci Approved for CI run label Nov 24, 2023
@geier1993
Copy link
Contributor Author

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (ffdee81) 62.85% compared to head (29a19c9) 62.19%.
Report is 60 commits behind head on develop.

Files Patch % Lines
src/eckit/utils/Translator.h 0.00% 2 Missing ⚠️
Additional details and impacted files

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

I don't think the tool is correct. The templated lines (https://app.codecov.io/gh/ecmwf/eckit/pull/78?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ecmwf#fa32d6530b7a2107fff999390cf9a157-R39) are used by eckit::Resource....

I added a further test that uses it as well

@geier1993 geier1993 added the approved-for-ci Approved for CI run label Nov 24, 2023
@simondsmart simondsmart merged commit 6919b50 into ecmwf:develop Dec 6, 2023
132 of 147 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants