SPQR: Cleanup for templates. #355
Conversation
Remove duplicate documentation at template specializations or explicit instantiations. Keep documentation only at template declaration or definition. Explicitly instantiate, forward declare, or specialize templates in a more consistent order.
SPQR/Include/SuiteSparseQR.hpp
Outdated
There was a problem hiding this comment.
Why are the comments removed from SPQR/Include/SuiteSparseQR.hpp? I really don't want to delete them.
There was a problem hiding this comment.
Like I tried to describe in the comment to the PR, I only removed duplicate inline documentation.
E.g., the inline documentation for SuiteSparseQR_numeric is still here:
https://github.com/mmuetzel/SuiteSparse/blob/112289734be3f6de6ef8aa3bb5eeb701d0612fc9/SPQR/Include/SuiteSparseQR.hpp#L653
There was a problem hiding this comment.
Also for this file -- there are lots of comments removed that shoudn't be removed.
There was a problem hiding this comment.
Which ones specifically do you mean?
| @@ -237,84 +237,27 @@ void spqrgpu_computeFrontStaging | |||
| return; | |||
| } | |||
|
|
|||
| template void spqrgpu_computeFrontStaging | |||
There was a problem hiding this comment.
I can't have these comments removed.
There was a problem hiding this comment.
They are still here: https://github.com/mmuetzel/SuiteSparse/blob/112289734be3f6de6ef8aa3bb5eeb701d0612fc9/SPQR/Include/spqrgpu.hpp#L28
That is the header that is installed for the user. And, imho, it's more important to keep them there.
They've just very recently been copied to this file. Imho, having to search through all files in case you'd like to update the comments for some reason will be hard to remember...
| template void spqr_parallel <double, int32_t> | ||
| ( | ||
| int32_t ntasks, |
There was a problem hiding this comment.
These changes are also spurious and not my style
There was a problem hiding this comment.
Is there a style-guide? What would be the correct style?
Imho, these explicit instantiations of templates are just a technicality and can be short. That makes it easier to see on the same screen for which types a template is explicitly instantiated.
|
Just a side-note: I find it a bit disheartening that you closed this PR without giving me any chance to react on your comments... 😓 |
|
Sorry about that. I can reopen it to discuss it. I have lots of places in my codes where I replicate comments like this. If I change one, I just cut-and-paste to update the other. I find it easier to navigate the code that way. I realize it may look like more work to keep things straight, but it's just the way my brain works. For example, take a look at the GraphBLAS/Include/GraphBLAS.h file. It has a huge number of comments, and in particular comments in the declarations where each parameter is defined. Then in the corresponding source file, those declarations are replicated (usually exactly). I just cut-and-paste between them to keep them up to date. I want to see them in both places so I can follow what I did. If I ever change an API signature, I can just cut-and-paste to get the different copies. If I want to do that for an SPQR template definition, I would then have to actually do more work to delete the comments (in addition to changing parameter types of some parameters). If I delete comments, the list of parameters is packed together on one line, and I find it harder (and thus more error prone) to hunt for the parameters that I have to change. I also keep the parameter names, even though it's not necessary either. It's just harder to edit if I compact it too far. The only time I do this (at least in my current style) is when I decide to declare a function "historical" which means "I will keep this for backward compatibilty, as long as possible, but recommend another method instead, and I delete it from any discussion in the user guide". So it's not quite deprecated, since I will keep it forever (unless it has a hard conflict with a feature in the future). |
|
Thanks for clarifying. I didn't expect that approach. It's quite different to other projects. But it's good to know. If I remember, I'll try to distill the couple of changes that are more than just comments and open a new PR for those. |
|
Thanks. Yes, I realize it's a bit unusual but it's a process that works well for my workflow. |
The recently added templates are very effective when it comes to using similar code for 32-bit and 64-bit indices.
Currently, the inline documentation at the function header is repeated (up to 5 times) at the template definition, at the template specializations, at the explicit template instantiations and/or at forward declarations.
That means that one would need to remember to change the documentation at all of these locations (sometimes scattered across different files) if one likes to keep it consistent.
The repeated documentation doesn't add value. Having it in one location is good enough imho.
This PR removes the repeated inline documentation at the template specializations, the explicit template instantiations and forward declarations. It keeps it only at the template definition.
Where the documentation is removed, a more compact form of the argument list is used.
I also tried to use a more consistent order for the different instantiations and specializations.
This is only house-keeping without any functional change.
There are only a few files where there are some syntax changes. E.g., in
spqrgpu_buildAssemblyMaps.cpp,spqrgpu_computeFrontStaging.cpp, orspqrgpu_kernel.cppwhere the template declarations were changed to explicit template instantiations.