improve loplugin rewriter double source modification detection

because my new rewriter easily generates overlapping rewriting.

Move the code from flatten and salcall up into the pluginhandler, and
drop the simpler detection logic.

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

6 files changed, 34 insertions, 68 deletions

diff --git a/compilerplugins/clang/flatten.cxx b/compilerplugins/clang/flatten.cxx
index 88156201c3ae..dd116d7a4ea5 100644
--- a/compilerplugins/clang/flatten.cxx
+++ b/compilerplugins/clang/flatten.cxx
@@ -49,13 +49,11 @@ private:
     SourceRange extendOverComments(SourceRange range);
     std::string getSourceAsString(SourceRange range);
     std::string invertCondition(Expr const * condExpr, SourceRange conditionRange);
-    bool checkOverlap(SourceRange);
     bool isLargeCompoundStmt(Stmt const *);
 
     Stmt const * lastStmtInCompoundStmt = nullptr;
     FunctionDecl const * functionDecl = nullptr;
     CompoundStmt const * functionDeclBody = nullptr;
-    std::vector<std::pair<char const *, char const *>> mvModifiedRanges;
     Stmt const * mElseBranch = nullptr;
 };
 
@@ -283,11 +281,6 @@ bool Flatten::rewrite1(IfStmt const * ifStmt)
     if (!rewriter)
         return false;
 
-    // If we overlap with a previous area we modified, we cannot perform this change
-    // without corrupting the source
-    if (!checkOverlap(ifStmt->getSourceRange()))
-        return false;
-
     auto conditionRange = ignoreMacroExpansions(ifStmt->getCond()->getSourceRange());
     if (!conditionRange.isValid()) {
         return false;
@@ -341,11 +334,6 @@ bool Flatten::rewrite2(IfStmt const * ifStmt)
     if (!rewriter)
         return false;
 
-    // If we overlap with a previous area we modified, we cannot perform this change
-    // without corrupting the source
-    if (!checkOverlap(ifStmt->getSourceRange()))
-        return false;
-
     auto conditionRange = ignoreMacroExpansions(ifStmt->getCond()->getSourceRange());
     if (!conditionRange.isValid()) {
         return false;
@@ -388,11 +376,6 @@ bool Flatten::rewriteLargeIf(IfStmt const * ifStmt)
     if (!rewriter)
         return false;
 
-    // If we overlap with a previous area we modified, we cannot perform this change
-    // without corrupting the source
-    if (!checkOverlap(ifStmt->getSourceRange()))
-        return false;
-
     auto conditionRange = ignoreMacroExpansions(ifStmt->getCond()->getSourceRange());
     if (!conditionRange.isValid()) {
         return false;
@@ -428,24 +411,6 @@ bool Flatten::rewriteLargeIf(IfStmt const * ifStmt)
     return true;
 }
 
-// If we overlap with a previous area we modified, we cannot perform this change
-// without corrupting the source
-bool Flatten::checkOverlap(SourceRange range)
-{
-    SourceManager& SM = compiler.getSourceManager();
-    char const  *p1 = SM.getCharacterData( range.getBegin() );
-    char const *p2 = SM.getCharacterData( range.getEnd() );
-    for (std::pair<char const *, char const *> const & rPair : mvModifiedRanges)
-    {
-        if (rPair.first <= p1 && p1 <= rPair.second)
-            return false;
-        if (p1 <= rPair.second && rPair.first <= p2)
-            return false;
-    }
-    mvModifiedRanges.emplace_back(p1, p2);
-    return true;
-}
-
 std::string Flatten::invertCondition(Expr const * condExpr, SourceRange conditionRange)
 {
     std::string s = getSourceAsString(conditionRange);
diff --git a/compilerplugins/clang/plugin.cxx b/compilerplugins/clang/plugin.cxx
index 2e9e390b6714..ef2b7667de85 100644
--- a/compilerplugins/clang/plugin.cxx
+++ b/compilerplugins/clang/plugin.cxx
@@ -399,6 +399,7 @@ bool RewritePlugin::insertText( SourceLocation Loc, StringRef Str, bool InsertAf
         return false;
     if( rewriter->InsertText( Loc, Str, InsertAfter, indentNewLines ))
         return reportEditFailure( Loc );
+    handler.addSourceModification(SourceRange(Loc, Loc.getLocWithOffset(Str.size())));
     return true;
 }
 
@@ -409,6 +410,7 @@ bool RewritePlugin::insertTextAfter( SourceLocation Loc, StringRef Str )
         return false;
     if( rewriter->InsertTextAfter( Loc, Str ))
         return reportEditFailure( Loc );
+    handler.addSourceModification(SourceRange(Loc, Loc.getLocWithOffset(Str.size())));
     return true;
 }
 
@@ -419,6 +421,7 @@ bool RewritePlugin::insertTextAfterToken( SourceLocation Loc, StringRef Str )
         return false;
     if( rewriter->InsertTextAfterToken( Loc, Str ))
         return reportEditFailure( Loc );
+    handler.addSourceModification(SourceRange(Loc, Loc.getLocWithOffset(Str.size())));
     return true;
 }
 
@@ -429,6 +432,7 @@ bool RewritePlugin::insertTextBefore( SourceLocation Loc, StringRef Str )
         return false;
     if( rewriter->InsertTextBefore( Loc, Str ))
         return reportEditFailure( Loc );
+    handler.addSourceModification(SourceRange(Loc, Loc.getLocWithOffset(Str.size())));
     return true;
 }
 
@@ -450,7 +454,7 @@ bool RewritePlugin::removeText( CharSourceRange range, RewriteOptions opts )
         return false;
     if( rewriter->getRangeSize( range, opts ) == -1 )
         return reportEditFailure( range.getBegin());
-    if( !handler.addRemoval( range.getBegin() ) )
+    if( !handler.checkOverlap( range.getAsRange() ) )
     {
         report( DiagnosticsEngine::Warning, "double code removal, possible plugin error", range.getBegin());
         return true;
@@ -462,6 +466,7 @@ bool RewritePlugin::removeText( CharSourceRange range, RewriteOptions opts )
     }
     if( rewriter->RemoveText( range, opts ))
         return reportEditFailure( range.getBegin());
+    handler.addSourceModification(range.getAsRange());
     return true;
 }
 
@@ -511,13 +516,15 @@ bool RewritePlugin::replaceText( SourceLocation Start, unsigned OrigLength, Stri
     assert( rewriter );
     if (wouldRewriteWorkdir(Start))
         return false;
-    if( OrigLength != 0 && !handler.addRemoval( Start ) )
+    SourceRange Range(Start, Start.getLocWithOffset(std::max<size_t>(OrigLength, NewStr.size())));
+    if( OrigLength != 0 && !handler.checkOverlap( Range ) )
     {
         report( DiagnosticsEngine::Warning, "double code replacement, possible plugin error", Start );
         return true;
     }
     if( rewriter->ReplaceText( Start, OrigLength, NewStr ))
         return reportEditFailure( Start );
+    handler.addSourceModification(Range);
     return true;
 }
 
@@ -528,13 +535,14 @@ bool RewritePlugin::replaceText( SourceRange range, StringRef NewStr )
         return false;
     if( rewriter->getRangeSize( range ) == -1 )
         return reportEditFailure( range.getBegin());
-    if( !handler.addRemoval( range.getBegin() ) )
+    if( !handler.checkOverlap( range ) )
     {
         report( DiagnosticsEngine::Warning, "double code replacement, possible plugin error", range.getBegin());
         return true;
     }
     if( rewriter->ReplaceText( range, NewStr ))
         return reportEditFailure( range.getBegin());
+    handler.addSourceModification(range);
     return true;
 }
 
@@ -545,13 +553,14 @@ bool RewritePlugin::replaceText( SourceRange range, SourceRange replacementRange
         return false;
     if( rewriter->getRangeSize( range ) == -1 )
         return reportEditFailure( range.getBegin());
-    if( !handler.addRemoval( range.getBegin() ) )
+    if( !handler.checkOverlap( range ) )
     {
         report( DiagnosticsEngine::Warning, "double code replacement, possible plugin error", range.getBegin());
         return true;
     }
     if( rewriter->ReplaceText( range, replacementRange ))
         return reportEditFailure( range.getBegin());
+    handler.addSourceModification(range);
     return true;
 }
 
diff --git a/compilerplugins/clang/plugin.hxx b/compilerplugins/clang/plugin.hxx
index c6f1c7cbd6e8..95983c00060d 100644
--- a/compilerplugins/clang/plugin.hxx
+++ b/compilerplugins/clang/plugin.hxx
@@ -21,6 +21,7 @@
 #include <clang/Frontend/CompilerInstance.h>
 #include <clang/Lex/Preprocessor.h>
 #include <unordered_map>
+#include <vector>
 
 #include <clang/Rewrite/Core/Rewriter.h>
 
diff --git a/compilerplugins/clang/pluginhandler.cxx b/compilerplugins/clang/pluginhandler.cxx
index 150bc4d1ef4d..3e78030993be 100644
--- a/compilerplugins/clang/pluginhandler.cxx
+++ b/compilerplugins/clang/pluginhandler.cxx
@@ -236,9 +236,22 @@ bool PluginHandler::checkIgnoreLocation(SourceLocation loc)
     return true;
 }
 
-bool PluginHandler::addRemoval( SourceLocation loc )
+// If we overlap with a previous area we modified, we cannot perform this change
+// without corrupting the source
+bool PluginHandler::checkOverlap(SourceRange range)
 {
-    return removals.insert( loc ).second;
+    SourceManager& SM = compiler.getSourceManager();
+    char const  *p1 = SM.getCharacterData( range.getBegin() );
+    char const *p2 = SM.getCharacterData( range.getEnd() );
+    for (std::pair<char const *, char const *> const & rPair : mvModifiedRanges)
+    {
+        if (rPair.first <= p1 && p1 <= rPair.second)
+            return false;
+        if (p1 <= rPair.second && rPair.first <= p2)
+            return false;
+    }
+    mvModifiedRanges.emplace_back(p1, p2);
+    return true;
 }
 
 void PluginHandler::HandleTranslationUnit( ASTContext& context )
diff --git a/compilerplugins/clang/pluginhandler.hxx b/compilerplugins/clang/pluginhandler.hxx
index cb75f9443bb5..05e8ce3502c7 100644
--- a/compilerplugins/clang/pluginhandler.hxx
+++ b/compilerplugins/clang/pluginhandler.hxx
@@ -54,10 +54,13 @@ public:
     DiagnosticBuilder report( DiagnosticsEngine::Level level, const char * plugin, StringRef message,
             CompilerInstance& compiler, SourceLocation loc = SourceLocation());
     bool ignoreLocation(SourceLocation loc);
-    bool addRemoval( SourceLocation loc );
     bool isDebugMode() const { return debugMode; }
     bool isLOOLMode() const { return !loolBasePath.empty(); }
     static bool isUnitTestMode();
+    // If we overlap with a previous area we modified, we cannot perform this change
+    // without corrupting the source
+    bool checkOverlap(SourceRange range);
+    bool addSourceModification(SourceRange range);
 private:
     void handleOption( const std::string& option );
     void createPlugins( std::set< std::string > rewriters );
@@ -67,12 +70,12 @@ private:
     StringRef const mainFileName;
     std::unordered_map<SourceLocation, bool> ignored_;
     Rewriter rewriter;
-    std::set< SourceLocation > removals;
     std::string scope;
     std::string warningsOnly;
     std::string loolBasePath;
     bool warningsAsErrors;
     bool debugMode = false;
+    std::vector<std::pair<char const*, char const*>> mvModifiedRanges;
 };
 
 /**
diff --git a/compilerplugins/clang/salcall.cxx b/compilerplugins/clang/salcall.cxx
index 2c33008ae276..2f289033851d 100644
--- a/compilerplugins/clang/salcall.cxx
+++ b/compilerplugins/clang/salcall.cxx
@@ -77,7 +77,6 @@ public:
 private:
     void checkForFunctionDecl(Expr const*, bool bCheckOnly = false);
     bool rewrite(SourceLocation);
-    bool checkOverlap(SourceRange);
     bool isSalCallFunction(FunctionDecl const* functionDecl, SourceLocation* pLoc = nullptr);
 
     std::set<FunctionDecl const*> m_addressOfSet;
@@ -87,7 +86,6 @@ private:
         Warning
     };
     PluginPhase m_phase;
-    std::vector<std::pair<char const*, char const*>> mvModifiedRanges;
 };
 
 bool SalCall::VisitUnaryAddrOf(UnaryOperator const* op)
@@ -662,35 +660,12 @@ bool SalCall::rewrite(SourceLocation locBegin)
 
     SourceRange range(locBegin, locEnd);
 
-    // If we overlap with a previous area we modified, we cannot perform this change
-    // without corrupting the source
-    if (!checkOverlap(range))
-        return false;
-
     if (!replaceText(locBegin, 9, ""))
         return false;
 
     return true;
 }
 
-// If we overlap with a previous area we modified, we cannot perform this change
-// without corrupting the source
-bool SalCall::checkOverlap(SourceRange range)
-{
-    SourceManager& SM = compiler.getSourceManager();
-    char const* p1 = SM.getCharacterData(range.getBegin());
-    char const* p2 = SM.getCharacterData(range.getEnd());
-    for (std::pair<char const*, char const*> const& rPair : mvModifiedRanges)
-    {
-        if (rPair.first <= p1 && p1 <= rPair.second)
-            return false;
-        if (p1 <= rPair.second && rPair.first <= p2)
-            return false;
-    }
-    mvModifiedRanges.emplace_back(p1, p2);
-    return true;
-}
-
 static loplugin::Plugin::Registration<SalCall> reg("salcall", true);
 }