Skip to content

DOC: Update documentation of GetNameOfClass macro calls#4477

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:masterfrom
N-Dekker:DOC-GetNameOfClass-macro-calls
Mar 23, 2024
Merged

DOC: Update documentation of GetNameOfClass macro calls#4477
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:masterfrom
N-Dekker:DOC-GetNameOfClass-macro-calls

Conversation

@N-Dekker
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker commented Feb 23, 2024


🎉 Ready for review now: improved documentation of 1000+ GetNameOfClass macro calls!

@github-actions github-actions Bot added the area:Core Issues affecting the Core module label Feb 23, 2024
Comment thread Modules/Core/Common/include/itkCellInterface.h Outdated
@N-Dekker N-Dekker force-pushed the DOC-GetNameOfClass-macro-calls branch from d1b702d to 9e00b62 Compare February 25, 2024 09:59
Comment thread Modules/Core/Common/include/itkCellInterface.h Outdated
@N-Dekker N-Dekker force-pushed the DOC-GetNameOfClass-macro-calls branch from 9e00b62 to 740b6ac Compare March 1, 2024 17:12
@github-actions github-actions Bot added area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Bridge Issues affecting the Bridge module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module area:Numerics Issues affecting the Numerics module labels Mar 1, 2024
@N-Dekker N-Dekker changed the title WIP: DOC: [no ci] Document GetNameOfClass macro calls DOC: Update documentation of GetNameOfClass macro calls Mar 1, 2024
@N-Dekker N-Dekker marked this pull request as ready for review March 1, 2024 17:15
@N-Dekker N-Dekker force-pushed the DOC-GetNameOfClass-macro-calls branch from 740b6ac to b1d2e87 Compare March 1, 2024 17:30
@github-actions github-actions Bot added the type:Documentation Documentation improvement or change label Mar 1, 2024
Copy link
Copy Markdown
Member

@blowekamp blowekamp left a comment

Choose a reason for hiding this comment

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

The /** */ comment is for doxygen. Restating the functions signature in the doxygen string is redundant and does not contribute to describing the API, IMHO.

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Mar 7, 2024

The /** */ comment is for doxygen. Restating the functions signature in the doxygen string is redundant and does not contribute to describing the API, IMHO.

Thanks @blowekamp, but did you look at the doxygen output, as it is currently generated? For example at https://itk.org/Doxygen/html/classitk_1_1LightObject.html

itk::LightObject::itkVirtualGetNameOfClassMacro(LightObject)
Return the name of this class as a string....

And at https://itk.org/Doxygen/html/classitk_1_1Object.html

itk::Object::itkOverrideGetNameOfClassMacro(Object)
Standard part of all itk objects.

So the doxygen output does not yet show function signature correctly! It does not expand those macro's! So it looks like restating the function signatures (as currently proposed) is not entirely useless 🤷

If you still don't like the current proposal, would you like it better if we simply just remove those lines of comment?

@N-Dekker N-Dekker marked this pull request as draft March 7, 2024 16:34
@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Mar 7, 2024

See also:

Let's postpone this pull request (4477) until the doxygen output at https://itk.org/Doxygen/html for those GetNameOfClass() member functions is fixed.

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Mar 8, 2024

Now that the doxygen output is OK again, it does indeed properly show the function signature of those GetNameOfClass() member functions, at https://itk.org/Doxygen/html/ Nevertheless, the current HTML documentation still seems outdated to me, as it just says something like "Standard part of all itk objects":

const char* itk::Object::GetNameOfClass() const [override] [virtual]
Standard part of all itk objects.

Would you like it if it would be replaced by the text "Returns the name of this class"? Or would you prefer no text at all (as the name of those member functions is already pretty clear)?

@blowekamp
Copy link
Copy Markdown
Member

Getting the function signature is a great improvement! Thank you.

Adding some prose would be nice. To me the interesting things about the method is that 1) Can be used polymorpically to return the name of the instantiated class 2) it return's the non-mangle and non-namespace qualified class name. To me the old "Run-time type information" says that to me, but I have been looked at that for far too long.

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Mar 8, 2024

Adding some prose would be nice. To me the interesting things about the method is that 1) Can be used polymorpically to return the name of the instantiated class 2) it return's the non-mangle and non-namespace qualified class name. To me the old "Run-time type information" says that to me, but I have been looked at that for far too long.

Thanks for the input @blowekamp Minor detail: GetNameOfClass() often returns the name of the class template, rather than the name of the class. For example, it may return "Image", without its template arguments, rather than "Image<int>".

Anyway, I would still like to keep the documentation very short for most of those 1000+member functions. Maybe we can only just have some more extensive documentation at LightObject::GetNameOfClass(), as LightObject is kind of the root of the ITK class hierarchy. And then just have the other GetNameOfClass() member functions refer to LightObject? So what about just \see LightObject::GetNameOfClass(), for any GetNameOfClass() other than the one of LightObject?

For example:

    /** \see LightObject::GetNameOfClass() */
    itkVirtualGetNameOfClassMacro(CellInterface);

@blowekamp
Copy link
Copy Markdown
Member

Yes very good idea.

Actually, if I recall correctly with Doxygen if an overloaded method has no documentation strings, then the parent's documentation is used. Last I checked that was before the override specifier was added so I don't know the behavior with that additional specifier. I would hope it would be the same.

@N-Dekker N-Dekker force-pushed the DOC-GetNameOfClass-macro-calls branch from b1d2e87 to 43be95d Compare March 9, 2024 13:38
Made the documentation of the `GetNameOfClass()` member functions more specific,
by referring to `LightObject`. Using Notepad++, Replace in Files:

  Find what: /\*\* .+ \*/\r\n  itkOverrideGetNameOfClassMacro
  Replace with: /\*\* \\see LightObject::GetNameOfClass\(\) \*/\r\n  itkOverrideGetNameOfClassMacro
  Find what: /\*\* .+ \*/\r\n  itkVirtualGetNameOfClassMacro
  Replace with: /\*\* \\see LightObject::GetNameOfClass\(\) \*/\r\n  itkVirtualGetNameOfClassMacro
  Filters: itk*.hxx;itk*.h;itk*.cxx
  [v] Match case
  (*) Regular expression

Follow-up to pull request InsightSoftwareConsortium#4373
commit 2c264ea
"STYLE: Replace itkTypeMacro calls with `itkOverrideGetNameOfClassMacro`"
commit fa1f39a
"STYLE: Replace itkTypeMacroNoParent with itkVirtualGetNameOfClassMacro"
@N-Dekker N-Dekker force-pushed the DOC-GetNameOfClass-macro-calls branch from 43be95d to e332901 Compare March 9, 2024 13:45
@N-Dekker N-Dekker marked this pull request as ready for review March 9, 2024 13:49
@N-Dekker
Copy link
Copy Markdown
Contributor Author

I really hope that this pull request can get merged without merge conflicts. I found that amending a commit like this (involving more than a thousand files) is quite time consuming! Just running git to do the commit locally takes a long time!

@blowekamp blowekamp self-requested a review March 15, 2024 12:59
@hjmjohnson hjmjohnson merged commit 4d75072 into InsightSoftwareConsortium:master Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Bridge Issues affecting the Bridge module 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:Python wrapping Python bindings for a class area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module type:Documentation Documentation improvement or change 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