InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.4k stars 663 forks source link

DOC: Update documentation of `GetNameOfClass` macro calls #4477

Closed N-Dekker closed 5 months ago

N-Dekker commented 6 months ago

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

N-Dekker commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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!