search

casacore / casarest

The remainder of the AIPS++ libraries that did not end up on casacore
GNU General Public License v3.0
4 stars 12 forks source link

Voronkov ma casacore 3.4 conversion #42

Closed gijzelaerr closed 3 years ago

aroffringa commented 3 years ago

I've merged all casacore 3.2/3.4 branches and rebased this branch onto master. This has reduced this MR quite nicely :). The remaining changes look fine to me, but maybe @VoronkovMA can look at those last two issues.

I also removed a couple includes from this branch which seemed unnecessary or incorrectly placed (see my last commit on this branch), maybe @VoronkovMA could give this branch a last check if it fixes everything for him too.

aroffringa commented 3 years ago

PS: @VoronkovMA also good to know might be that there exists a function casacore::makeVector (you wrote a new function block2vector which does the same thing). I've therefore undone your lwimager changes.

VoronkovMA commented 3 years ago

@aroffringa thanks, your changes make sense. I only wonder why you reverted C++ style type cast (static_cast(0)) to C-style (ssize_t(0)) in synthesis/MeasurementEquations/ConvolutionEquation.cc, although in this particular case it doesn't make a difference. I can confirm that I can build this branch fine in my environment (and the code I am trying to build at the moment which depends on casarest). This build brought 3 additional compiler warnings about missing return value in private methods verifyScaleSizes, updateRHS and writeMatrixToDisk in synthesis/MeasurementEquations/MultiTermMatrixCleaner.cc. I either overlooked this before or this was introduced by other merges you've done. Based on the code, I suggest a trivial change of making these 3 methods to return void instead of Int (they're private and return value is not expected anywhere by the looks of it + no documentation on what the return value is supposed to be). I tried doing it in my environment and it doesn't seem to cause any problems. But I was unable to push these changes to the server due to lack of permissions.

tammojan commented 3 years ago

@VoronkovMA You should now have write access to this repository. As you're probably used to, please stick to the usual workflow, i.e. have a pull request reviewed by someone else and don't commit to the main branch directly.

aroffringa commented 3 years ago

@VoronkovMA I find static_cast<ssize_t>(0) way too verbose for having a value of zero, since there's no way you can incorrectly convert 0 (moreover, my compiler already warns when I would do something like ssize_t(1e15) ). Google C++ style does discourages ssize_t(0) though, so I've changed it to brace initialization (ssize_t{0}). I removed the warnings.

This code is compiled without -Wall; it would probably be a good idea to use -Wall, since there are many issues with the code that -Wall will identify (like lots of virtual exceptions by value). However, we don't use casarest anymore, so we'll no longer really maintain it.

VoronkovMA commented 3 years ago

thanks, @tammojan. It looks like @aroffringa make all the changes I had in mind. I tried to build the current master (after merge) as an extra sanity check and it worked fine. Are you going to tag the release 1.8 any time soon?