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

Reorder histogram template parameters #1800

Merged

Conversation

danhoeflinger
Copy link
Contributor

Reorder histogram template parameters and remove unused default template argument.

One option for specification of histogram (version 1a from email).

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>
@danhoeflinger danhoeflinger added this to the 2022.7.0 milestone Sep 4, 2024
@danhoeflinger
Copy link
Contributor Author

After offline discussion, this version was chosen, as it was determined that we will almost surely break no code by reversing these two template arguments.
If we have some report of bug / break, we have an option in the future to add code to handle deprecated case which requires explicit specification of arguments, but the first response would be to instead instruct users remove their explicit template arguments. If they require explicit specification of types to force some conversion, we can instruct them to add an explicit conversion in addition to removing the specified template types.

Copy link
Contributor

@mmichel11 mmichel11 left a comment

Choose a reason for hiding this comment

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

LGTM

@adamfidel
Copy link
Contributor

Based on the discussion today, LGTM.

Copy link
Contributor

@rarutyun rarutyun left a comment

Choose a reason for hiding this comment

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

The changes look exactly how we agreed on. Looks good to me!

@danhoeflinger danhoeflinger merged commit 0414309 into main Sep 4, 2024
21 checks passed
@danhoeflinger danhoeflinger deleted the dev/dhoeflin/histogram_template_parameter_order branch September 4, 2024 20:52
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.

5 participants