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

STYLE: Change template parameter name prefix N to V #3171

Merged

Conversation

Leengit
Copy link
Contributor

@Leengit Leengit commented Feb 8, 2022

Template parameters that are of typename have names that start with T, but for template parameters that are values (e.g., unsigned int) rather than types, both N and V have been used as a prefix for their names. This pull request changes instances with prefix N to have a V prefix. Additionally NDimensions (plural) is changed to VDimension (singular).

Note that class members (typically static const(expr) members) are sometimes named with an N or V prefix. Perhaps neither of these prefixes is appropriate by the ITK Style Guide. Nonetheless, these are not changed because outside code, such as ITK modules, might refer to them by their current names.

Thanks to @N-Dekker for discovering this issue.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

@github-actions github-actions bot added area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module area:Video Issues affecting the Video module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Style Style changes: no logic impact (indentation, comments, naming) type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Feb 8, 2022
@N-Dekker
Copy link
Contributor

N-Dekker commented Feb 8, 2022

Great job @Leengit 👍 Thanks for addressing the issue that I raised at discussion topic Which initial is preferred for a non-type template parameter, N or V (NDimension or VDimension)?

Do you think we should then also adjust Utilities/KWStyle/ITK.kws.xml? I don't really know how that works, but the xml code <Template>([TNV]... seems to allow both N and V.

Some minor comments regarding your commit messages:

BUG: Revert b04d9cd for member variables.

I'm not sure if that particular SHA (b04d9cd) will make it onto the master branch. During rebase/merge, the SHA of the commit may change. It's probably more safe to just say something like:

BUG: Revert changes from previous commit for member variables

But I guess you might as well squash both commits to one, if that's OK to you.

I noticed that you add a period ('.') to the subject line of commit messages. Is that really helpful to you? Because it seems a bit odd to me. A large majority of commit messages certainly don't have that period. And it does not seem necessary to me, because it is just the subject, not necessarily a complete English sentence. And as the number of characters preferred for a subject line is limited (maximum 72 character), I wouldn't want to start adding periods to my subject lines!

@Leengit Leengit marked this pull request as draft February 8, 2022 13:58
@Leengit
Copy link
Contributor Author

Leengit commented Feb 8, 2022

Thank you @N-Dekker for the suggestions. With this force-push, I have merged the commits, removed the SHA code and trailing period from the commit message, and removed N from Utilities/KWStyle/ITK.kws.xml.

Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Looks good on a glance. There were also quite some manual interventions here!

@Leengit Leengit changed the title STYLE: Change N... template parameters to V.... STYLE: Change template parameter name prefix N to V Feb 8, 2022
@Leengit Leengit marked this pull request as ready for review February 8, 2022 20:54
@Leengit
Copy link
Contributor Author

Leengit commented Feb 8, 2022

I'd like to see how this does with overnight testing runs because it touches a lot of files. If all remains green then this should be good to merge.

Copy link
Contributor

@N-Dekker N-Dekker left a comment

Choose a reason for hiding this comment

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

Approved, even though personally I would have preferred consistent use of the N, rather than the V as prefix for non-type template parameters.

@thewtex
Copy link
Member

thewtex commented Feb 10, 2022

Please rebase and add the commit to .git-blame-ignore-revs.

@Leengit
Copy link
Contributor Author

Leengit commented Feb 10, 2022

Rebased. Will add to .git-blame-ignore-revs after merging.

@Leengit
Copy link
Contributor Author

Leengit commented Feb 11, 2022

This passed all its tests. I have squashed, rebased to current master, and force-pushed. This is ready for merging.

@kwrobot-v1
Copy link

kwrobot-v1 bot commented Feb 11, 2022

Errors:

  • Failed to reserve ref 123fa9d for the merge request: invalid git ref: 'no such commit'.
  • Failed to run the checks: mr utilities error: failed to list commits of 123fa9dfcd327998ecf75dc0b52c4dbe090178e0 for https://github.com/InsightSoftwareConsortium/ITK/pull/3171: fatal: bad object 123fa9dfcd327998ecf75dc0b52c4dbe090178e0 .

@Leengit
Copy link
Contributor Author

Leengit commented Feb 11, 2022

I rebased against the newly current master and force-pushed. The strange message from kwrobot-v1 makes it seem like my force-push collided with a merge attempt by someone else. If that's what is going on, attempting the merge again might be the right course of action. If not, please help me to understand the message from kwrobot-v1.

@dzenanz
Copy link
Member

dzenanz commented Feb 11, 2022

The KWRobot message came 1 second after my approval, so I think that is what caused it. Your PR is not merged.

@Leengit
Copy link
Contributor Author

Leengit commented Feb 11, 2022

The KWRobot message came 1 second after my approval, so I think that is what caused it. Your PR is not merged.

Probably then my force-push collided with your approval; perhaps KWRobot was trying to say that you were approving a commit that had been obsoleted in the seconds before-hand.

Perhaps extra caution is appropriate nonetheless. We might wait for the checks to pass (again) before merging.

@dzenanz
Copy link
Member

dzenanz commented Feb 11, 2022

Your force-push was 5 minutes before my approval. So no, I can't imagine those two colliding.

@Leengit
Copy link
Contributor Author

Leengit commented Feb 11, 2022

There was a second force push.

@dzenanz
Copy link
Member

dzenanz commented Feb 11, 2022

Than that was probably it. I now see that force-push, but I didn't see it when I wrote #3171 (comment).

STYLE: Remove N as acceptable template parameter name prefix
@Leengit
Copy link
Contributor Author

Leengit commented Feb 14, 2022

Because one of the checks was stalled for several days, I just took the opportunity to rebase to the current master branch and force push. There are no other changes.

@dzenanz dzenanz merged commit 4b43536 into InsightSoftwareConsortium:master Feb 14, 2022
@dzenanz dzenanz deleted the template_parameter_naming branch February 14, 2022 18:44
@dzenanz
Copy link
Member

dzenanz commented Feb 14, 2022

This is probably what broke FEM module. It is off by default. I have it enabled locally, for testing. I now run into:

96>------ Build started: Project: ITKFEMRegistration, Configuration: Debug x64 ------
96>itkFEMRegistrationFilter.cxx
96>C:\Dev\ITK-git\Modules\Numerics\FEM\include\itkImageToRectilinearFEMObjectFilter.h(72,54): error C2065: 'VDimension': undeclared identifier
96>C:\Dev\ITK-git\Modules\Numerics\FEM\include\itkImageToRectilinearFEMObjectFilter.h(175): message : see reference to class template instantiation 'itk::fem::ImageToRectilinearFEMObjectFilter<TInputImage>' being compiled
96>C:\Dev\ITK-git\Modules\Numerics\FEM\include\itkImageToRectilinearFEMObjectFilter.h(72,1): error C2975: 'VDimension': invalid template argument for 'itk::fem::FEMObject', expected compile-time constant expression
96>C:\Dev\ITK-git\Modules\Numerics\FEM\include\itkFEMObject.h(74): message : see declaration of 'VDimension'
95>ITKIOHDF5.vcxproj -> C:\Dev\ITK-vs19\lib\Debug\ITKIOHDF5-5.3.lib
96>Done building project "ITKFEMRegistration.vcxproj" -- FAILED.

@@ -69,7 +69,7 @@ class ITK_TEMPLATE_EXPORT ImageToRectilinearFEMObjectFilter : public ProcessObje
using ImageIndexType = typename InputImageType::IndexType;

/** Typedefs for Output FEMObject */
using FEMObjectType = typename itk::fem::FEMObject<NDimensions>;
using FEMObjectType = typename itk::fem::FEMObject<VDimension>;
Copy link
Member

Choose a reason for hiding this comment

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

Definition of this on line 60 was not updated.

Copy link
Member

Choose a reason for hiding this comment

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

Addressed in #3201.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module area:Video Issues affecting the Video module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Style Style changes: no logic impact (indentation, comments, naming) type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants