Bug 120773 - core/dbaccess/source/ui/browser/sbagrid.cxx:1265: possible typo in if expression ?
Summary: core/dbaccess/source/ui/browser/sbagrid.cxx:1265: possible typo in if express...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Julien Nabet
URL:
Whiteboard: target:6.2.0
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-22 07:43 UTC by dcb314
Modified: 2018-10-23 05:18 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description dcb314 2018-10-22 07:43:01 UTC 
core/dbaccess/source/ui/browser/sbagrid.cxx:1265]: (style) Same expression on both sides of '||'.

Source code is

        if ((nCol == BROWSER_INVALIDID) || (nRow >= nCorrectRowCount) || nCol == 0  || nCol == BROWSER_INVALIDID )

Maybe better code

        if ((nCol == BROWSER_INVALIDID) || (nRow >= nCorrectRowCount) || nCol == 0  || nRow == BROWSER_INVALIDID )
Comment 1 Alex Thurgood 2018-10-22 07:45:40 UTC 
Not being a coder, I have no idea, but I can refer to the experts...
Comment 2 Alex Thurgood 2018-10-22 07:46:27 UTC 
@Lionel, Julien : thoughts ?
Comment 3 Julien Nabet 2018-10-22 08:10:24 UTC 
I don't know how this part works, it could also be:
if ((nCol == BROWSER_INVALIDID) || (nRow >= nCorrectRowCount) || nCol == 0 )
Comment 4 Lionel Elie Mamane 2018-10-22 08:49:04 UTC 
This comes from my commit

commit 2d73671e930a0099c26e268a2be417bfcdd7fc91
Author: Lionel Elie Mamane <lionel@mamane.lu>
Date:   Thu Oct 4 19:33:14 2018 +0200

    clean up column pos vs column id
    
    everywhere GetColumnAtXPosPixel is used;
    see also tdf#119564 fix
    
    Change-Id: Ibab57c7305bf4dce9ea9f3df66e6214b0d1585b2


You'll notice that around line 1240-1245, I changed:

  nCol = GetColumnAtXPosPixel(rEvt.maPosPixel.X());
to
  nCol = GetColumnId(GetColumnAtXPosPixel(rEvt.maPosPixel.X()));

So, the discussed line was created by search/replace of "GetColumnId(nCol)" by "nCol" :)

Just remove the last (duplicate) || clause. The whole thing is safe because GetColumnId(BROWSER_INVALIDID) == BROWSER_INVALIDID (see svtools/source/brwbox/brwbox2.cxx around line 375), so for any x:
  x == BROWSER_INVALIDID implies GetColumnId(x) == BROWSER_INVALIDID
and in particular
  x == BROWSER_INVALIDID || GetColumnId(x) == BROWSER_INVALIDID
is equivalent to
  GetColumnId(x) == BROWSER_INVALIDID

Here, x is the old value of nCol (before my patch), namely
  GetColumnAtXPosPixel(rEvt.maPosPixel.X())
Comment 5 Julien Nabet 2018-10-22 21:12:50 UTC 
https://gerrit.libreoffice.org/#/c/62206/
Comment 6 Commit Notification 2018-10-23 05:18:15 UTC 
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9deabca91c8fd899fd162f4a16a1061793e8a10e

tdf#120773: simplify condition in sbagrid.cxx:1265

It will be available in 6.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.