Details
-
Type:
Improvement
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: astshim
-
Labels:
-
Story Points:0.5
-
Epic Link:
-
Sprint:AP F18-2
-
Team:Alert Production
Description
While working on DM-11971 I found that CmpMap was missing an override of copyPolymorphic. Improve the unit tests so this would be caught.
Consider removing the definition of copyPolymporphic and copy from Mapping, since that class is abstract. This may make it easier to detect omission in subclasses of Mapping, since such code will not compile. However, that still leaves deeper classes that could forget to override it, so unit testing is more important.
Finally, remove the virtual since it is implied by override.
Consider adding explicit @copydoc to copyPolymorphic, though Doxygen already shows the documentation.
Attachments
Issue Links
- is blocked by
-
DM-11971 memory leak in astshim isSeries function
- Done
Activity
It looks like you made no changes to the unit tests, including tests of copy. Was that intentional?
Krzysztof Findeisen
added a comment - It looks like you made no changes to the unit tests, including tests of copy . Was that intentional? Yes. I guess my previous comment was not clear enough. What I meant to say is that the existing tests of copy seem to be adequate to show the problem. So this turned into a more minor cleanup ticket.
In that case, I've no objections to merging.
Krzysztof Findeisen
added a comment - In that case, I've no objections to merging.
It is not possible to test copyPolymorphic directly but testing copy shows the error and is being done.
I removed copyPolymorphic from Mapping, making it a virtual class. That required one trival change in functional.cc. I also removed redundant virtual keywords.