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/49493Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

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);

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;
 }
 

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>
 

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 )

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;
 };
 
 /**

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);
 }