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

refactor: template implementation #79342

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Brambor
Copy link
Contributor

@Brambor Brambor commented Jan 25, 2025

Summary

None

Purpose of change

There are two almost identical methods differing only in the tripoint type they operate on.

Less code duplication is better.

Describe the solution

Template them.

Do so only in the implementation (.cpp, not .h)

Describe alternatives you've considered

Do it in .h file too. This would require to move the implementation to the .h file.

Testing

Compiles.

Additional context

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Jan 25, 2025
@Brambor
Copy link
Contributor Author

Brambor commented Jan 25, 2025

If @PatrikLundell says this is no good, then it is no good. They made so much work on typifying and that is very appreciated!

Copy link
Contributor

@PatrikLundell PatrikLundell left a comment

Choose a reason for hiding this comment

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

In most places I've seen, the placeholder template class is called Tripoint (uppercase) or Point, so I think it makes sense to follow that convention unless there's a reason not to.

Also note that this only makes sense for relative points, and I think "north" etc. is defined only for those.

I know you can put constraints on which types to accept, and I think it would be good to do so here to constrain the valid matches to only relative types. If nothing else, it servers as very cryptic documentation.
I don't think it would make sense to restrain the template to tripoints, as it would work perfectly fine with (relative) points. I think I've seen Tripoint being used without any dimension constraints to (I assume) serve as an indication of the intended usage, so here I think usage of Tripoint or Point would be a matter of taste (again, if there's no specific reason for the use of "tripo").

General comment outside the scope of the review.
As an aside, yes, I very much prefer having user facing "real" operations with the templates hidden in the implementation. From my perspective, it serves a number of purposes:

  • It tells the user what operations are available, rather than hard to decode templates, typically bunched up among other templates.
  • It tells the compiler what's available, so it won't do stupid things like getting confused when someone does something wrong that matches more than one template, resulting at best in horrible compiler complaint barfs that have to be deciphered, or, at worst, linker errors with no clues at all.
  • It tells the compiler what's available, so it can cleanly tell the user that there's no match (at least VS' Intellisense is template incompetent, so it frequently (always?) won't mark mismatches).

@Brambor Brambor marked this pull request as draft January 25, 2025 17:18
@Brambor
Copy link
Contributor Author

Brambor commented Jan 25, 2025

Thanks for the review! I will improve the implementation later. I am in the middle of debugging NPC trade problems.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants