Repository navigation
Make compatible with casacore 3.8.2 - #18
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The declared CMake minimum does not support C++20, and making the public storage-manager class final introduces an unrelated breaking API change.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Updates LofarStMan and its tests for casacore 3.8.2 compatibility.
Changes:
- Replaces legacy casacore aliases with standard C++ types.
- Modernizes virtual overrides and column accessors.
- Raises the minimum CMake version.
| File | Description |
|---|---|
CMakeLists.txt |
Updates the minimum CMake version. |
include/LofarStMan/LofarStMan.h |
Modernizes storage-manager declarations and types. |
include/LofarStMan/LofarColumn.h |
Updates column interfaces and overrides. |
src/LofarStMan.cc |
Applies compatible types throughout storage management. |
src/LofarColumn.cc |
Updates column implementations to standard types. |
test/tLofarStMan.cc |
Adapts functional tests to the updated APIs. |
test/tIOPerf.cc |
Updates I/O performance tests and string handling. |
test/tfix.cc |
Modernizes the column-fix utility test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Some of the |
This PR is just a compatibility fix. Feel free to try that yourself ;). |
I do think that changing |
Thanks! Some of this was also discussed in mail, but I think that replacing all ints by int32_t isn't what we want either. I agree that anything used for storage should have a fixed width, but we shouldn't want to have fixed size int32_ts in the API or be used in every loop, etc. --- that's not why the int32_t type exists, that's why int exists. It's not that replacing Int by int suddenly makes this a problem, as they are equivalent. It's not harder to do a replace-all from int to int32_t than it is to replace all Int to int32_t, if we really did would have wanted that. I had also asked Marcel to review, I'll await what he thinks before merging :). |
gmloose
left a comment
gmloose
left a comment
There was a problem hiding this comment.
Just two minor issues that don't stand in the way, I think.
| ArrayColumn<Complex> dataCol(tab, "DATA"); | ||
| ArrayColumn<float> weightCol(tab, "WEIGHT"); | ||
| ArrayColumn<float> wspecCol(tab, "WEIGHT_SPECTRUM"); | ||
| ArrayColumn<float> sigmaCol(tab, "SIGMA"); | ||
| ArrayColumn<double> uvwCol(tab, "UVW"); | ||
| ArrayColumn<bool> flagCol(tab, "FLAG"); | ||
| ArrayColumn<bool> flagcatCol(tab, "FLAG_CATEGORY"); | ||
| ScalarColumn<double> timeCol(tab, "TIME"); | ||
| ScalarColumn<double> centCol(tab, "TIME_CENTROID"); | ||
| ScalarColumn<double> intvCol(tab, "INTERVAL"); | ||
| ScalarColumn<double> expoCol(tab, "EXPOSURE"); | ||
| ScalarColumn<int> ant1Col(tab, "ANTENNA1"); | ||
| ScalarColumn<int> ant2Col(tab, "ANTENNA2"); | ||
| ScalarColumn<int> feed1Col(tab, "FEED1"); | ||
| ScalarColumn<int> feed2Col(tab, "FEED2"); | ||
| ScalarColumn<int> ddidCol(tab, "DATA_DESC_ID"); | ||
| ScalarColumn<int> pridCol(tab, "PROCESSOR_ID"); | ||
| ScalarColumn<int> fldidCol(tab, "FIELD_ID"); | ||
| ScalarColumn<int> arridCol(tab, "ARRAY_ID"); | ||
| ScalarColumn<int> obsidCol(tab, "OBSERVATION_ID"); | ||
| ScalarColumn<int> stidCol(tab, "STATE_ID"); | ||
| ScalarColumn<int> scnrCol(tab, "SCAN_NUMBER"); | ||
| ScalarColumn<bool> flagrowCol(tab, "FLAG_ROW"); |
There was a problem hiding this comment.
Huh, did that class type change name? Or did you fix a lingering bug?
There was a problem hiding this comment.
Yes, the Column classes that start with "RO" have been deprecated quite long ago, and are aliased to their non-RO counter parts. When they were deprecated, the C++ deprecated annotation probably didn't exist yet ;) . But we could now also make the aliases [[deprecated()]].
|
|
||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -O3") | ||
| add_compile_options( | ||
| -O3 |
There was a problem hiding this comment.
In general, I think it's a bad idea to hard-code optimization levels in a CMakeLists.txt file, but since this is just reformatting ..., I'll ignore it.
There was a problem hiding this comment.
I think what we in general want is that the default is -O3, even when not explicitly specifying it is a release. If that can be done otherwise I'm fine with that, though the hardcoded -O3 is I think in almost all our software ;).
If you want we can discuss further, for now I'll merge as indeed the -O3 behaviour itself didn't change.
Thanks!
No description provided.