loplugin:simplifybool, check for !(!a op !b)

Change-Id: Ic3ee9c05705817580633506498f848aac3ab7ba6
Reviewed-on: https://gerrit.libreoffice.org/67866
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

21 files changed, 96 insertions, 45 deletions

diff --git a/compilerplugins/clang/simplifybool.cxx b/compilerplugins/clang/simplifybool.cxx
index b4752b4108aa..40c5b6fc48ba 100644
--- a/compilerplugins/clang/simplifybool.cxx
+++ b/compilerplugins/clang/simplifybool.cxx
@@ -177,20 +177,47 @@ bool SimplifyBool::VisitUnaryLNot(UnaryOperator const * expr) {
         // triggers.
         if (compat::getBeginLoc(binaryOp).isMacroID())
             return true;
-        if (!binaryOp->isComparisonOp())
-            return true;
-        auto t = binaryOp->getLHS()->IgnoreImpCasts()->getType()->getUnqualifiedDesugaredType();
-        if (t->isTemplateTypeParmType() || t->isDependentType() || t->isRecordType())
-            return true;
-        // for floating point (with NaN) !(x<y) need not be equivalent to x>=y
-        if (t->isFloatingType() ||
-            binaryOp->getRHS()->IgnoreImpCasts()->getType()->getUnqualifiedDesugaredType()->isFloatingType())
-            return true;
-        report(
-            DiagnosticsEngine::Warning,
-            ("logical negation of comparison operator, can be simplified by inverting operator"),
-            compat::getBeginLoc(expr))
-            << expr->getSourceRange();
+        if (binaryOp->isComparisonOp())
+        {
+            auto t = binaryOp->getLHS()->IgnoreImpCasts()->getType()->getUnqualifiedDesugaredType();
+            if (t->isTemplateTypeParmType() || t->isDependentType() || t->isRecordType())
+                return true;
+            // for floating point (with NaN) !(x<y) need not be equivalent to x>=y
+            if (t->isFloatingType() ||
+                binaryOp->getRHS()->IgnoreImpCasts()->getType()->getUnqualifiedDesugaredType()->isFloatingType())
+                return true;
+            report(
+                DiagnosticsEngine::Warning,
+                ("logical negation of comparison operator, can be simplified by inverting operator"),
+                compat::getBeginLoc(expr))
+                << expr->getSourceRange();
+        }
+        else if (binaryOp->isLogicalOp())
+        {
+            auto containsNegation = [](Expr const * expr) {
+                expr = ignoreParenImpCastAndComma(expr);
+                if (auto unaryOp = dyn_cast<UnaryOperator>(expr))
+                    if (unaryOp->getOpcode() == UO_LNot)
+                        return expr;
+                if (auto binaryOp = dyn_cast<BinaryOperator>(expr))
+                    if (binaryOp->getOpcode() == BO_NE)
+                        return expr;
+                if (auto cxxOpCall = dyn_cast<CXXOperatorCallExpr>(expr))
+                    if (cxxOpCall->getOperator() == OO_ExclaimEqual)
+                        return expr;
+                return (Expr const*)nullptr;
+            };
+            auto lhs = containsNegation(binaryOp->getLHS());
+            auto rhs = containsNegation(binaryOp->getRHS());
+            if (!lhs || !rhs)
+                return true;
+            if (lhs || rhs)
+                report(
+                    DiagnosticsEngine::Warning,
+                    ("logical negation of logical op containing negation, can be simplified"),
+                    compat::getBeginLoc(binaryOp))
+                    << binaryOp->getSourceRange();
+        }
     }
     if (auto binaryOp = dyn_cast<CXXOperatorCallExpr>(expr->getSubExpr()->IgnoreParenImpCasts())) {
         // Ignore macros, otherwise
diff --git a/compilerplugins/clang/test/simplifybool.cxx b/compilerplugins/clang/test/simplifybool.cxx
index 01549f320ab0..64288253c8e2 100644
--- a/compilerplugins/clang/test/simplifybool.cxx
+++ b/compilerplugins/clang/test/simplifybool.cxx
@@ -9,6 +9,8 @@
 
 #include <rtl/ustring.hxx>
 
+namespace group1
+{
 void f1(int a, int b)
 {
     if (!(a < b))
@@ -25,9 +27,11 @@ void f2(float a, float b)
         a = b;
     }
 };
+};
 
 // Consistently either warn about all or none of the below occurrences of "!!":
-
+namespace group2
+{
 enum E1
 {
     E1_1 = 1
@@ -57,9 +61,11 @@ bool f1(E1 e) { return !!(e & E1_1); }
 bool f2(E2 e) { return !!(e & E2_1); }
 
 bool f3(E3 e) { return !!(e & E3::E1); }
+};
 
 // record types
-
+namespace group3
+{
 struct Record1
 {
     bool operator==(const Record1&) const;
@@ -113,5 +119,25 @@ struct Record4
         return v;
     }
 };
+};
+
+namespace group4
+{
+bool foo1(bool a, bool b)
+{
+    return !(!a && !b);
+    // expected-error@-1 {{logical negation of logical op containing negation, can be simplified [loplugin:simplifybool]}}
+}
+bool foo2(int a, bool b)
+{
+    return !(a != 1 && !b);
+    // expected-error@-1 {{logical negation of logical op containing negation, can be simplified [loplugin:simplifybool]}}
+}
+bool foo3(int a, bool b)
+{
+    // no warning expected
+    return !(a != 1 && b);
+}
+};
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/dbaccess/source/ui/querydesign/SelectionBrowseBox.cxx b/dbaccess/source/ui/querydesign/SelectionBrowseBox.cxx
index 070e2ab2814f..766b1dcd2523 100644
--- a/dbaccess/source/ui/querydesign/SelectionBrowseBox.cxx
+++ b/dbaccess/source/ui/querydesign/SelectionBrowseBox.cxx
@@ -65,7 +65,7 @@ namespace
 {
     bool isFieldNameAsterisk(const OUString& _sFieldName )
     {
-        bool bAsterisk = !(!_sFieldName.isEmpty() && _sFieldName.toChar() != '*');
+        bool bAsterisk = _sFieldName.isEmpty() || _sFieldName.toChar() == '*';
         if ( !bAsterisk )
         {
             sal_Int32 nTokenCount = comphelper::string::getTokenCount(_sFieldName, '.');
diff --git a/idlc/source/options.cxx b/idlc/source/options.cxx
index 04db926dad0d..4f49cd202de4 100644
--- a/idlc/source/options.cxx
+++ b/idlc/source/options.cxx
@@ -227,7 +227,7 @@ bool Options::initOptions(std::vector< std::string > & rArgs)
     {
     case 'O':
       {
-        if (!((++first != last) && ((*first)[0] != '-')))
+        if ((++first == last) || ((*first)[0] == '-'))
         {
           return badOption("invalid", option);
         }
@@ -237,7 +237,7 @@ bool Options::initOptions(std::vector< std::string > & rArgs)
       }
     case 'M':
       {
-        if (!((++first != last) && ((*first)[0] != '-')))
+        if ((++first == last) || ((*first)[0] == '-'))
         {
           return badOption("invalid", option);
         }
@@ -247,7 +247,7 @@ bool Options::initOptions(std::vector< std::string > & rArgs)
       }
     case 'I':
       {
-        if (!((++first != last) && ((*first)[0] != '-')))
+        if ((++first == last) || ((*first)[0] == '-'))
         {
           return badOption("invalid", option);
         }
@@ -283,7 +283,7 @@ bool Options::initOptions(std::vector< std::string > & rArgs)
       }
     case 'D':
       {
-        if (!((++first != last) && ((*first)[0] != '-')))
+        if ((++first == last) || ((*first)[0] == '-'))
         {
           return badOption("invalid", option);
         }
diff --git a/sc/source/filter/xml/xmlsubti.cxx b/sc/source/filter/xml/xmlsubti.cxx
index d28a930b2030..1dea576a3e75 100644
--- a/sc/source/filter/xml/xmlsubti.cxx
+++ b/sc/source/filter/xml/xmlsubti.cxx
@@ -243,12 +243,12 @@ uno::Reference< drawing::XShapes > const & ScMyTables::GetCurrentXShapes()
 
 bool ScMyTables::HasDrawPage()
 {
-    return !((maCurrentCellPos.Tab() != nCurrentDrawPage) || !xDrawPage.is());
+    return (maCurrentCellPos.Tab() == nCurrentDrawPage) && xDrawPage.is();
 }
 
 bool ScMyTables::HasXShapes()
 {
-    return !((maCurrentCellPos.Tab() != nCurrentXShapes) || !xShapes.is());
+    return (maCurrentCellPos.Tab() == nCurrentXShapes) && xShapes.is();
 }
 
 void ScMyTables::AddOLE(const uno::Reference <drawing::XShape>& rShape,
diff --git a/sd/source/core/CustomAnimationEffect.cxx b/sd/source/core/CustomAnimationEffect.cxx
index 4bda45524403..418a52b35e05 100644
--- a/sd/source/core/CustomAnimationEffect.cxx
+++ b/sd/source/core/CustomAnimationEffect.cxx
@@ -717,7 +717,7 @@ void CustomAnimationEffect::setTargetSubItem( sal_Int16 nSubItem )
 
 void CustomAnimationEffect::setDuration( double fDuration )
 {
-    if( !((mfDuration != -1.0) && (mfDuration != fDuration)) )
+    if( (mfDuration == -1.0) || (mfDuration == fDuration) )
         return;
 
     try
diff --git a/sd/source/core/stlsheet.cxx b/sd/source/core/stlsheet.cxx
index 83aff4e2fb13..81b465775e8c 100644
--- a/sd/source/core/stlsheet.cxx
+++ b/sd/source/core/stlsheet.cxx
@@ -713,7 +713,7 @@ void SAL_CALL SdStyleSheet::release(  ) throw ()
 void SAL_CALL SdStyleSheet::dispose(  )
 {
     ClearableMutexGuard aGuard( mrBHelper.rMutex );
-    if (!(!mrBHelper.bDisposed && !mrBHelper.bInDispose))
+    if (mrBHelper.bDisposed || mrBHelper.bInDispose)
         return;
 
     mrBHelper.bInDispose = true;
diff --git a/sd/source/ui/dlg/tpoption.cxx b/sd/source/ui/dlg/tpoption.cxx
index 84b8563928f0..406c8552d42d 100644
--- a/sd/source/ui/dlg/tpoption.cxx
+++ b/sd/source/ui/dlg/tpoption.cxx
@@ -343,7 +343,7 @@ void SdTpOptionsMisc::ActivatePage( const SfxItemSet& rSet )
     SetFieldUnit( *m_pMtrFldOriginalHeight, eFUnit, true );
     m_pMtrFldOriginalHeight->SetValue( m_pMtrFldOriginalHeight->Normalize( nVal ), FieldUnit::TWIP );
 
-    if( !(nWidth != 0 && nHeight != 0) )
+    if( nWidth == 0 || nHeight == 0 )
         return;
 
     m_pMtrFldInfo1->SetUnit( eFUnit );
diff --git a/sd/source/ui/slidesorter/controller/SlsCurrentSlideManager.cxx b/sd/source/ui/slidesorter/controller/SlsCurrentSlideManager.cxx
index fd926c732638..d3e21aff80bd 100644
--- a/sd/source/ui/slidesorter/controller/SlsCurrentSlideManager.cxx
+++ b/sd/source/ui/slidesorter/controller/SlsCurrentSlideManager.cxx
@@ -120,7 +120,7 @@ void CurrentSlideManager::SwitchCurrentSlide (
     const SharedPageDescriptor& rpDescriptor,
     const bool bUpdateSelection)
 {
-    if (!(rpDescriptor.get() != nullptr && mpCurrentSlide!=rpDescriptor))
+    if (rpDescriptor.get() == nullptr || mpCurrentSlide==rpDescriptor)
         return;
 
     ReleaseCurrentSlide();
diff --git a/sd/source/ui/view/DocumentRenderer.cxx b/sd/source/ui/view/DocumentRenderer.cxx
index bf23f1a30cdb..149e328e7022 100644
--- a/sd/source/ui/view/DocumentRenderer.cxx
+++ b/sd/source/ui/view/DocumentRenderer.cxx
@@ -1375,9 +1375,8 @@ private:
 
         PrintInfo aInfo (mpPrinter, mpOptions->IsPrintMarkedOnly());
 
-        if (!(aInfo.mpPrinter!=nullptr && pShell!=nullptr))
-return;
-
+        if (aInfo.mpPrinter==nullptr || pShell==nullptr)
+            return;
 
         MapMode aMap (aInfo.mpPrinter->GetMapMode());
         aMap.SetMapUnit(MapUnit::Map100thMM);
diff --git a/sd/source/ui/view/Outliner.cxx b/sd/source/ui/view/Outliner.cxx
index 0c98800946b0..e86136bb79a4 100644
--- a/sd/source/ui/view/Outliner.cxx
+++ b/sd/source/ui/view/Outliner.cxx
@@ -1331,7 +1331,7 @@ void SdOutliner::SetViewMode (PageKind ePageKind)
     std::shared_ptr<sd::ViewShell> pViewShell (mpWeakViewShell.lock());
     std::shared_ptr<sd::DrawViewShell> pDrawViewShell(
         std::dynamic_pointer_cast<sd::DrawViewShell>(pViewShell));
-    if (!(pDrawViewShell != nullptr && ePageKind != pDrawViewShell->GetPageKind()))
+    if (pDrawViewShell == nullptr || ePageKind == pDrawViewShell->GetPageKind())
         return;
 
     // Restore old edit mode.
diff --git a/sd/source/ui/view/ViewShellBase.cxx b/sd/source/ui/view/ViewShellBase.cxx
index 260afc29d8fe..4bc8cc834387 100644
--- a/sd/source/ui/view/ViewShellBase.cxx
+++ b/sd/source/ui/view/ViewShellBase.cxx
@@ -829,7 +829,7 @@ void ViewShellBase::UpdateBorder ( bool bForce /* = false */ )
     // We have to check the existence of the window, too.
     // The SfxViewFrame accesses the window without checking it.
     ViewShell* pMainViewShell = GetMainViewShell().get();
-    if (!(pMainViewShell != nullptr && GetWindow()!=nullptr))
+    if (pMainViewShell == nullptr || GetWindow()==nullptr)
         return;
 
     SvBorder aCurrentBorder (GetBorderPixel());
diff --git a/sdext/source/presenter/PresenterScrollBar.cxx b/sdext/source/presenter/PresenterScrollBar.cxx
index bd156861c44f..9be3b86776e2 100644
--- a/sdext/source/presenter/PresenterScrollBar.cxx
+++ b/sdext/source/presenter/PresenterScrollBar.cxx
@@ -185,7 +185,7 @@ void PresenterScrollBar::SetThumbPosition (
 {
     nPosition = ValidateThumbPosition(nPosition);
 
-    if (!(nPosition != mnThumbPosition && ! mbIsNotificationActive))
+    if (nPosition == mnThumbPosition || mbIsNotificationActive)
         return;
 
     mnThumbPosition = nPosition;
diff --git a/sdext/source/presenter/PresenterTextView.cxx b/sdext/source/presenter/PresenterTextView.cxx
index 4feb8d92ad21..eb995dd935b3 100644
--- a/sdext/source/presenter/PresenterTextView.cxx
+++ b/sdext/source/presenter/PresenterTextView.cxx
@@ -1096,8 +1096,8 @@ void PresenterTextCaret::SetPosition (
     const sal_Int32 nParagraphIndex,
     const sal_Int32 nCharacterIndex)
 {
-    if (!(mnParagraphIndex != nParagraphIndex
-        || mnCharacterIndex != nCharacterIndex))
+    if (mnParagraphIndex == nParagraphIndex
+        && mnCharacterIndex == nCharacterIndex)
         return;
 
     if (mnParagraphIndex >= 0)
diff --git a/sfx2/source/view/sfxbasecontroller.cxx b/sfx2/source/view/sfxbasecontroller.cxx
index 280529855614..4fdfe8b1a67e 100644
--- a/sfx2/source/view/sfxbasecontroller.cxx
+++ b/sfx2/source/view/sfxbasecontroller.cxx
@@ -1390,7 +1390,7 @@ void SfxBaseController::ShowInfoBars( )
             bIsGoogleFile = true;
     }
 
-    if ( !(!bCheckedOut && !bIsGoogleFile) )
+    if ( bCheckedOut || bIsGoogleFile )
         return;
 
     // Get the Frame and show the InfoBar if not checked out
diff --git a/svgio/source/svgreader/svgclippathnode.cxx b/svgio/source/svgreader/svgclippathnode.cxx
index 090a795da3ac..8d09b51db7ca 100644
--- a/svgio/source/svgreader/svgclippathnode.cxx
+++ b/svgio/source/svgreader/svgclippathnode.cxx
@@ -127,7 +127,7 @@ namespace svgio
             drawinglayer::primitive2d::Primitive2DContainer& rContent,
             const basegfx::B2DHomMatrix* pTransform) const
         {
-            if(!(!rContent.empty() && Display_none != getDisplay()))
+            if(rContent.empty() || Display_none == getDisplay())
                 return;
 
             const drawinglayer::geometry::ViewInformation2D aViewInformation2D;
diff --git a/svgio/source/svgreader/svgmasknode.cxx b/svgio/source/svgreader/svgmasknode.cxx
index f2a919c4582f..286a1088ab60 100644
--- a/svgio/source/svgreader/svgmasknode.cxx
+++ b/svgio/source/svgreader/svgmasknode.cxx
@@ -192,7 +192,7 @@ namespace svgio
             drawinglayer::primitive2d::Primitive2DContainer& rTarget,
             const basegfx::B2DHomMatrix* pTransform) const
         {
-            if(!(!rTarget.empty() && Display_none != getDisplay()))
+            if(rTarget.empty() || Display_none == getDisplay())
                 return;
 
             drawinglayer::primitive2d::Primitive2DContainer aMaskTarget;
diff --git a/svgio/source/svgreader/svgstylenode.cxx b/svgio/source/svgreader/svgstylenode.cxx
index 68a101fb9637..cb81941dbbda 100644
--- a/svgio/source/svgreader/svgstylenode.cxx
+++ b/svgio/source/svgreader/svgstylenode.cxx
@@ -147,7 +147,7 @@ namespace svgio
         {
             // aSelectors: possible comma-separated list of CssStyle definitions, no spaces at start/end
             // aContent: the svg style definitions as string
-            if(!(!aSelectors.isEmpty() && !aContent.isEmpty()))
+            if(aSelectors.isEmpty() || aContent.isEmpty())
                 return;
 
             // create new style and add to local list (for ownership control)
diff --git a/svtools/source/contnr/imivctl1.cxx b/svtools/source/contnr/imivctl1.cxx
index 064f923e425a..c8d689c1a729 100644
--- a/svtools/source/contnr/imivctl1.cxx
+++ b/svtools/source/contnr/imivctl1.cxx
@@ -1979,8 +1979,7 @@ void SvxIconChoiceCtrl_Impl::Command( const CommandEvent& rCEvt )
 
 void SvxIconChoiceCtrl_Impl::ToTop( SvxIconChoiceCtrlEntry* pEntry )
 {
-    if( !(!maZOrderList.empty()
-           && pEntry != maZOrderList.back()))
+    if( maZOrderList.empty() || pEntry == maZOrderList.back())
         return;
 
     auto it = std::find(maZOrderList.begin(), maZOrderList.end(), pEntry);
@@ -2698,7 +2697,7 @@ void SvxIconChoiceCtrl_Impl::InitSettings()
     pView->SetBackground( rStyleSettings.GetFieldColor());
 
     long nScrBarSize = rStyleSettings.GetScrollBarSize();
-    if( !(nScrBarSize != nHorSBarHeight || nScrBarSize != nVerSBarWidth) )
+    if( nScrBarSize == nHorSBarHeight && nScrBarSize == nVerSBarWidth )
         return;
 
     nHorSBarHeight = nScrBarSize;
diff --git a/sw/source/filter/basflt/shellio.cxx b/sw/source/filter/basflt/shellio.cxx
index b66cb249ba22..1c0d473cbd4c 100644
--- a/sw/source/filter/basflt/shellio.cxx
+++ b/sw/source/filter/basflt/shellio.cxx
@@ -644,7 +644,7 @@ bool SwReader::HasGlossaries( const Reader& rOptions )
 
     // if a Medium is selected, get its Stream
     bool bRet = false;
-    if( !( nullptr != (po->m_pMedium = pMedium ) && !po->SetStrmStgPtr() ))
+    if(  nullptr == (po->m_pMedium = pMedium ) || po->SetStrmStgPtr() )
         bRet = po->HasGlossaries();
     return bRet;
 }
@@ -660,7 +660,7 @@ bool SwReader::ReadGlossaries( const Reader& rOptions,
 
     // if a Medium is selected, get its Stream
     bool bRet = false;
-    if( !( nullptr != (po->m_pMedium = pMedium ) && !po->SetStrmStgPtr() ))
+    if( nullptr == (po->m_pMedium = pMedium ) || po->SetStrmStgPtr() )
         bRet = po->ReadGlossaries( rBlocks, bSaveRelFiles );
     return bRet;
 }
diff --git a/vcl/source/graphic/GraphicObject2.cxx b/vcl/source/graphic/GraphicObject2.cxx
index 7148c1098c87..7349e8b25c63 100644
--- a/vcl/source/graphic/GraphicObject2.cxx
+++ b/vcl/source/graphic/GraphicObject2.cxx
@@ -493,7 +493,7 @@ void GraphicObject::ImplTransformBitmap( BitmapEx&          rBmpEx,
 
     const Size  aSizePixel( rBmpEx.GetSizePixel() );
 
-    if( !(rAttr.GetRotation() != 0 && !IsAnimated()) )
+    if( rAttr.GetRotation() == 0 || IsAnimated() )
         return;
 
     if( !(aSizePixel.Width() && aSizePixel.Height() && rDstSize.Width() && rDstSize.Height()) )