CARTAvis / carta-backend

Source code repository for the backend component of CARTA, a new visualization tool designed for the ALMA, the VLA and the SKA pathfinders.
https://cartavis.github.io/
GNU General Public License v3.0
22 stars 11 forks source link

Fix empty name variable on RHEL9 #1251

Closed ajm-ska closed 1 year ago

ajm-ska commented 1 year ago

Description

This PR fixes #1250

It is similar to issue #1199 so I used the same idea to modify line 811 src/Region/CrtfImportExport.cc from

if (name.front() == '"' && name.back() == '"') {

to

if (!name.empty() && name.front() == '"' && name.back() == '"') {

With this change, the CASA_REGION_IMPORT_EXPORT test passes on RHEL9 again.

I notice there are similar lines in Ds9ImportExport.cc and RegionImportExport.cc, but I have not touched them as there are no other issues with the ICD tests at present. I'm not sure if there could be potential problems in future when the use of the newer GCC (>11.2.1) becomes more common?

Checklist

ajm-ska commented 1 year ago

@kswang1029 Do you think this warrants a changelog entry?

kswang1029 commented 1 year ago

@kswang1029 Do you think this warrants a changelog entry?

No changlog is needed I think.

confluence commented 1 year ago

I notice there are similar lines in Ds9ImportExport.cc and RegionImportExport.cc, but I have not touched them as there are no other issues with the ICD tests at present.

I think that use of front / back without a check that the string is not empty is a logic error which is bound to cause problems later, and we should fix this everywhere while we're fixing it here.

ajm-ska commented 1 year ago

I think that use of front / back without a check that the string is not empty is a logic error which is bound to cause problems later, and we should fix this everywhere while we're fixing it here.

OK. On closer inspection, the front() and back() call in Ds9ImportExport.cc can not be an empty string, so that file does not need modification: if (name.front() == '{' && name.back() == '}') {

However, RegionImportExport.cc does have the possibility to call empty strings, so I have applied the same ! .empty() logic fix to the appropriate instances in that file.

As far as I can see, there are no other instances in the source code where there could be calls to empty strings, so this should be the end of the problem.

I have tested the region import/export ICD tests on macOS and RHEL9 on this branch and they continue to pass.

confluence commented 1 year ago

On closer inspection, the front() and back() call in Ds9ImportExport.cc can not be an empty string

I'm not sure about this -- the usage seems directly analogous to the CRTF import/export file. The string is looked up in a map just like it is here, but that doesn't mean the value can't be empty.

github-actions[bot] commented 1 year ago

Code Coverage

Package Line Rate Health
src.Cache 68%
src.DataStream 52%
src.FileList 68%
src.Frame 51%
src.HttpServer 43%
src.ImageData 28%
src.ImageFitter 89%
src.ImageGenerators 52%
src.ImageStats 74%
src.Logger 44%
src.Main 54%
src.Region 19%
src.Session 30%
src.Table 52%
src.ThreadingManager 87%
src.Timer 85%
src.Util 50%
Summary 39% (6818 / 17596)
ajm-ska commented 1 year ago

I'm not sure about this -- the usage seems directly analogous to the CRTF import/export file. The string is looked up in a map just like it is here, but that doesn't mean the value can't be empty.

OK. I have added the same ! .empty() check to the line.

I have run the import/export ICD tests (they include DS9-related tests) on macOS and RHEL9 again and they continue to pass, so everything should be still fine.