libadm icon indicating copy to clipboard operation
libadm copied to clipboard

simplify auto_base with C++17 parameter pack expansion in using

Open tomjnixon opened this issue 2 years ago • 1 comments

todo:

  • [x] update the big comment
  • [x] revisit VariantParameter -- perhaps this can now be implemented without the virtual base
    • ~no, this is still required. one possible improvement would be to flatten the list of parameters in HasParametersImpl, so that HasParameters<A, HasParameters<B, C>> (as produced by VariantParameter) is turned into HasParameters<A, B, C>, producing fewer Combine classes.~
    • yeah, now that i understand how virtual inheritance works, it's obvious that it can be done with CRTP. parameter pack expansion still makes this much cleaner than would have been possible before
  • [x] check if the visibility still makes sense now that it's possible to change it all in one place
    • no, this is required because the template wrapper functions have the same names as the tag-dispatched functions
  • [x] try auto-detecting which methods to merge, as it may be simpler than using the Flags thing
    • no: VariantParameter uses HasParameters, so the has etc. methods are overloaded, which can't be detected without knowing the parameter types
  • [x] check impact on binary size and build time
    • shared debug
      • any build time difference is within measurement error for me
      • .so size with debug information: 39.5MB to 38.7MB
      • .so size stripped: 3.2MB to 3.1MB

tomjnixon avatar Jan 16 '24 17:01 tomjnixon

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (3f94a9b) to head (3642d45). Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
include/adm/detail/auto_base.hpp 77.77% 2 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #194      +/-   ##
==========================================
- Coverage   89.91%   89.90%   -0.02%     
==========================================
  Files         125      125              
  Lines        5812     5816       +4     
==========================================
+ Hits         5226     5229       +3     
- Misses        586      587       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jan 16 '24 18:01 codecov-commenter