[MapScript] Inconsistent return values (0/-1 vs MS_SUCCESS/FAILURE)

[dmorissette]

This is a problem that's been around for quite a while: some functions in MapScript return 0/-1, and others return MS_SUCCESS/MS_FAILURE.

We should eventually walk through the MapScript API and list all functions that still return 0/-1. Then for every function we should make required changes to return MS_SUCCESS/FAILURE and also make sure that we look for and update every location where the function is called.

We have talked about this between me and Steve, and it would be a good idea to clean this us before the 3.7 release if possible.

[dmorissette]

Whoever has time first between me, Steve, Assefa or Julien (hopefully Julien ;) 
should assign this bug to himself and walk through the MapScript API and 
identify all functions that return 0/-1 and should be returning 
MS_SUCCESS/FAILURE.

Then report the list of functions in the bug report and we'll split the job of 
fixing things.

[jlacroix]

I pass through mapscript.i and found many functions that doesn't return
MS_SUCCESS or MS_FAILURE. I report all this function here. 
The way I report them:

Class
  (mapscript function) => (mapserver function called) return value



Function list
-------------
mapObj
  addColor => msAddColor doesn't exist
  embedScalebar => msEmbedScalebar return 0 or -1
  embedLegend => msEmbedLegend return 0 or -1
  drawLabelCache => msDrawLabelCache return MS_SUCCESS or
                        msDrawLabelCacheGD return 0 or -1
                        msDrawLabelCacheSWF return 0 or -1
                        msDrawLabelCachePDF return 0 or -1
  setProjection => msLoadProjectionString return -1 or 
                     msProcessProjection return 0 or -1
  save => msSaveMap return 0 or -1
  setSymbolSet => msLoadSymbolSet return 0 or -1
  setFontSet => msLoadFontSet return 0 or -1
  moveLayerup => msMoveLayerUp return 0 or -1
  moveLayerdown => msMoveLayerDown return 0 or -1
  setLayersdrawingOrder => msSetLayersdrawingOrder return 1 or 0
layerObj
  draw => msDrawLayer return MS_SUCCESS or MS_FAILURE or
              msDrawRasterLayer return MS_FAILURE or
                  msDrawRasterLayerLow return 0 or -1
                  msDrawRasterLayerSWF return 0 or -1
                  msDrawRasterLayerPDF return 0 or -1
  drawQuery => msDrawLayer return MS_SUCCESS or MS_FAILURE or
                   msDrawRasterLayer return MS_FAILURE or
                       msDrawRasterLayerLow return 0 or -1
                       msDrawRasterLayerSWF return 0 or -1
                       msDrawRasterLayerPDF return 0 or -1
  setFilter => loadExpressionString return 0 or -1
  addFeature => return 0 or -1
classObj
  setExpression => loadExpressionString return 0 or -1
  setText => loadExpressionString return 0 or -1
pointObj
  draw => msDrawPoint return MS_SUCCESS or MS_FAILURE or 0
lineObj
  add => return 0 or -1
shapeObj
  add => msAddLine return 0 or -1
  copy => msCopyShape return 0 or -1
  contains => msIntersectPointPolygon return MS_FALSE or !MS_FALSE
  intersects => return -1 or
                    msIntersectPolylines return MS_FALSE or MS_TRUE
                    msIntersectPolygons return MS_FALSE or MS_TRUE
                    msIntersectPolylinePolygon return MS_[FALSE/TRUE]
rectObj
  contrain => msConstrainRect doesn't exist
  draw => return 0
shapefileObj
  get => return 0 or -1
  getPoint => return 0 or -1
  getTransformed => return 0 or -1
  add => msSHPWriteShape record number
  addPoint => msSHPWritePoint record number or -1
imageObj
  saveToString => need #if defined (SWIGPYTHON) || defined (SWIGTCL8)
DBFInfo
  getFieldType => msDBFGetFieldInfo return DBFFieldType

[dmorissette]

Definitely too late for 4.0... setting version to 4.1

[hobu]

Should we try and revisit this for 5.0? CC'ing Tamas...

[hobu]

This should be done as part of the MapServer C API effort (should that ever happen). Pushing it forward to 5.2...

[dmorissette]

This ticket was discussed at the TO Code Sprint and the consensus is to leave the existing functions unchanged, and come up with a RFC to provide guidelines for future functions (i.e. new methods should use MS_SUCCESS/MS_FAILURE instead of 0/-1)

[dmorissette]

Created RFC 53: ​http://www.mapserver.org/development/rfc/ms-rfc-53.html