About use of sal_IntPtr for FontList::GetSizeAry, aStdSizeAry and mpSizeAry (svtools)

Julien Nabet <serval2412@xxxxxxxx> · Thu, 10 Sep 2020 21:00:02 +0200



    Hello,
    IMHO the use of sal_IntPtr in FontList class is wrong.
    It's been there since 43135665bf9093c52f424069bcf83d50a93bdc0c:
    author    Fridrich Štrba <fridrich.strba@xxxxxxxxxx>   
      2013-06-10 12:49:33 +0200

      committer    Fridrich Štrba <fridrich.strba@xxxxxxxxxx>   
      2013-06-10 14:03:34 +0200

      commit    43135665bf9093c52f424069bcf83d50a93bdc0c (patch)

      tree    079359e37353f3a936656a9d3aeff2e3577cc353

      parent    378b2522b40004ca5f5b6de0b4eda0ac13d4153d (diff)

      mingw64: use integers able to contain a size in svtools

      Change-Id: Id5505f75a2331be682b74d085a7959fc4bf07df8

      
    Let's take a look at the 2 vars:

    
    1) aStdSizeAry
    it's a static array containing values from 0 to 960.
    (see
https://opengrok.libreoffice.org/xref/core/svtools/source/control/ctrltool.cxx?r=770ca016#38)
    2) mpSizeAry
    git grep -n mpSizeAry in core repo gives:

      include/svtools/ctrltool.hxx:149:                           
      mpSizeAry;

      svtools/source/control/ctrltool.cxx:749:    mpSizeAry.reset();

      svtools/source/control/ctrltool.cxx:772:    mpSizeAry.reset(new
      sal_IntPtr[nDevSizeCount+1] );

      svtools/source/control/ctrltool.cxx:779:           
      mpSizeAry[nRealCount] = nOldHeight;

      svtools/source/control/ctrltool.cxx:783:    mpSizeAry[nRealCount]
      = 0;

      svtools/source/control/ctrltool.cxx:786:    return
      mpSizeAry.get();

    
    so the elements are only initialized thanks to "nOldHeight" which
      itself is declared as a long and is initialized with
      "aSize.Height()"
    (see
https://opengrok.libreoffice.org/xref/core/svtools/source/control/ctrltool.cxx?r=770ca016#778).
    Indeed, according to include/tools/gen.hxx, "Height()" returns a
      long, so "nOldHeight" is ok.

    
    (see
https://opengrok.libreoffice.org/xref/core/include/tools/gen.hxx?r=32090b01#189)

    
    I know there has been some problem about types of attributes of
      Size, there's even been an attempt to use sal_Int32 but reverted
      with d36f7c5bd2115fcdd26ba8ff7b6a0446dea70bd4
    "Revert "long->sal_Int32 in tools/gen.hxx"
    This reverts commit 8bc951daf79decbd8a599a409c6d33c5456710e0.
      As discussed at
<https://lists.freedesktop.org/archives/libreoffice/2018-April/079955.html>
      "long->sal_Int32 in tools/gen.hxx", that commit caused lots of
      problems with
      signed integer overflow, and the original plan was to redo it to
      consistently
      use sal_Int64 instead of sal_Int32.
      <https://gerrit.libreoffice.org/#/c/52471/>
      "sal_Int32->sal_Int64 in tools/gen.hxx" tried that. However, it
      failed
      miserably on Windows, causing odd failures like not writing out
      Pictures/*.svm
      streams out into .odp during CppunitTest_sd_export_ooxml2. So the
      next best
      approach is to just revert the original commit, at least for now.
      Includes revert of follow-up 8c50aff2175e85c54957d98ce32af40a3a87e168
      "Fix
      Library_vclplug_qt5"."
    Anyway for the moment do you think we could change the use of
      sal_IntPtr for these quoted elements into long types?
    Julien

    
_______________________________________________
LibreOffice mailing list
LibreOffice@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/libreoffice