RFC remove the last argument LoaderBase* from Parser::Parse() function

[ollydbg]

Request for comments for days.
I'm going to apply this patch:

Code

83f55f3e1094289f7afb485921ef8f83e882cbec
 src/plugins/codecompletion/parser/parser.cpp | 19 +++++++++----------
 src/plugins/codecompletion/parser/parser.h   |  3 +--
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/src/plugins/codecompletion/parser/parser.cpp b/src/plugins/codecompletion/parser/parser.cpp
index c774ec9..1aef556 100644
--- a/src/plugins/codecompletion/parser/parser.cpp
+++ b/src/plugins/codecompletion/parser/parser.cpp
@@ -531,7 +531,7 @@ void Parser::AddParse(const wxString& filename)
     CC_LOCKER_TRACK_P_MTX_UNLOCK(ParserCommon::s_ParserMutex)
 }
 
-bool Parser::Parse(const wxString& filename, bool isLocal, bool locked, LoaderBase* loader)
+bool Parser::Parse(const wxString& filename, bool isLocal, bool locked)
 {
     ParserThreadOptions opts;
 
@@ -546,7 +546,7 @@ bool Parser::Parse(const wxString& filename, bool isLocal, bool locked, LoaderBa
 
     opts.storeDocumentation    = m_Options.storeDocumentation;
 
-    opts.loader                = loader; // maybe 0 at this point
+    opts.loader                = nullptr; // must be 0 at this point
 
     bool result = false;
     do
@@ -567,16 +567,15 @@ bool Parser::Parse(const wxString& filename, bool isLocal, bool locked, LoaderBa
 
         if (!canparse)
         {
-           if (opts.loader) // if a loader is already open at this point, the caller must clean it up
-               CCLogger::Get()->DebugLog(_T("Parser::Parse(): CodeCompletion Plugin: FileLoader memory leak ")
-                                         _T("while loading file ") + filename);
-           break;
+
+            CCLogger::Get()->DebugLog(_T("Parser::Parse(): file already parsed or reserved for parsing") + filename);
+            break;
         }
 
-        // this should always be true
-        // memory will leak if a loader has already been initialized before this point
-        if (!opts.loader)
-            opts.loader = Manager::Get()->GetFileManager()->Load(filename, m_NeedsReparse);
+        // once the Load function is called, it will return a loader pointer, and start loading
+        // the file content in a background thread(see: BackgroundThread class)
+        // the loader will be deleted in the ParserThread::InitTokenizer() function.
+        opts.loader = Manager::Get()->GetFileManager()->Load(filename, m_NeedsReparse);
 
         ParserThread* thread = new ParserThread(this, filename, isLocal, opts, m_TokenTree);
         TRACE(_T("Parser::Parse(): Parsing %s"), filename.wx_str());
diff --git a/src/plugins/codecompletion/parser/parser.h b/src/plugins/codecompletion/parser/parser.h
index edac352..d87db95 100644
--- a/src/plugins/codecompletion/parser/parser.h
+++ b/src/plugins/codecompletion/parser/parser.h
@@ -341,9 +341,8 @@ protected:
 
     /** parse the file, either immediately or delayed.
      * @param locked give the status of the Tokentree, false means the tree is not locked
-     * @param loader is the object to load the file to internally buffer (lower down to Tokenizer)
      */
-    bool Parse(const wxString& filename, bool isLocal = true, bool locked = false, LoaderBase* loader = NULL);
+    bool Parse(const wxString& filename, bool isLocal = true, bool locked = false);
 
     /** delete those files from the TokenTree, and add them again thought AddParse() function */
     void ReparseModifiedFiles();

This is going to remove the last parameter of Parser::Parse() function. It was the LoaderBase pointer, but this argument is always set as nullptr, otherwise, it will report a memory leak problem.

The loader must be allocated in the Parse() function, by calling the SDK's Load function:

Code

opts.loader = Manager::Get()->GetFileManager()->Load(filename, m_NeedsReparse);

and later the loader will be deleted in ParserThread::InitTokenizer() function.

[MortenMacFly]

I'm going to apply this patch:

As usual I cannot apply this GIT patch but from visual inspection it looks fine to me.

[ollydbg]

I'm going to apply this patch:

As usual I cannot apply this GIT patch but from visual inspection it looks fine to me.

Now, please try this patch:

Code

Index: src/plugins/codecompletion/parser/parser.cpp
===================================================================
--- src/plugins/codecompletion/parser/parser.cpp	(revision 9823)
+++ src/plugins/codecompletion/parser/parser.cpp	(working copy)
@@ -531,7 +531,7 @@ void Parser::AddParse(const wxString& filename)
     CC_LOCKER_TRACK_P_MTX_UNLOCK(ParserCommon::s_ParserMutex)
 }
 
-bool Parser::Parse(const wxString& filename, bool isLocal, bool locked, LoaderBase* loader)
+bool Parser::Parse(const wxString& filename, bool isLocal, bool locked)
 {
     ParserThreadOptions opts;
 
@@ -546,7 +546,7 @@ bool Parser::Parse(const wxString& filename, bool isLocal, bool locked, LoaderBa
 
     opts.storeDocumentation    = m_Options.storeDocumentation;
 
-    opts.loader                = loader; // maybe 0 at this point
+    opts.loader                = nullptr; // must be 0 at this point
 
     bool result = false;
     do
@@ -567,16 +567,15 @@ bool Parser::Parse(const wxString& filename, bool isLocal, bool locked, LoaderBa
 
         if (!canparse)
         {
-           if (opts.loader) // if a loader is already open at this point, the caller must clean it up
-               CCLogger::Get()->DebugLog(_T("Parser::Parse(): CodeCompletion Plugin: FileLoader memory leak ")
-                                         _T("while loading file ") + filename);
-           break;
+
+            CCLogger::Get()->DebugLog(_T("Parser::Parse(): file already parsed or reserved for parsing") + filename);
+            break;
         }
 
-        // this should always be true
-        // memory will leak if a loader has already been initialized before this point
-        if (!opts.loader)
-            opts.loader = Manager::Get()->GetFileManager()->Load(filename, m_NeedsReparse);
+        // once the Load function is called, it will return a loader pointer, and start loading
+        // the file content in a background thread(see: BackgroundThread class)
+        // the loader will be deleted in the ParserThread::InitTokenizer() function.
+        opts.loader = Manager::Get()->GetFileManager()->Load(filename, m_NeedsReparse);
 
         ParserThread* thread = new ParserThread(this, filename, isLocal, opts, m_TokenTree);
         TRACE(_T("Parser::Parse(): Parsing %s"), filename.wx_str());
Index: src/plugins/codecompletion/parser/parser.h
===================================================================
--- src/plugins/codecompletion/parser/parser.h	(revision 9823)
+++ src/plugins/codecompletion/parser/parser.h	(working copy)
@@ -341,9 +341,8 @@ protected:
 
     /** parse the file, either immediately or delayed.
      * @param locked give the status of the Tokentree, false means the tree is not locked
-     * @param loader is the object to load the file to internally buffer (lower down to Tokenizer)
      */
-    bool Parse(const wxString& filename, bool isLocal = true, bool locked = false, LoaderBase* loader = NULL);
+    bool Parse(const wxString& filename, bool isLocal = true, bool locked = false);
 
     /** delete those files from the TokenTree, and add them again thought AddParse() function */
     void ReparseModifiedFiles();

Note that I use a script to generate this SVN compatible patch under msysgit.

Code

#!/bin/bash
#
# git-svn-diff originally by (http://mojodna.net/2009/02/24/my-work-git-workflow.html)
# modified by mike@mikepearce.net
# modified by aconway@[redacted] - handle diffs that introduce new files
# modified by t.broyer@ltgt.net - fixes diffs that introduce new files
# modified by m@rkj.me - fix sed syntax issue in OS X
# modified by dertom - fix new file-bug. Whitespace mandatory for new file '+++ file.c'
#
# Generate an SVN-compatible diff against the tip of the tracking branch

# Get the tracking branch (if we're on a branch)
# TRACKING_BRANCH=`git svn info | grep URL | sed -e 's/.*\/branches\///'`


# If the tracking branch has 'URL' at the beginning, then the sed wasn't successful and
# we'll fall back to the svn-remote config option
#if [[ "$TRACKING_BRANCH" =~ URL.* ]]
#then
TRACKING_BRANCH=`git config --get svn-remote.svn.fetch | sed -e 's/.*:refs\/remotes\///'`
#fi

# Get the highest revision number
REV=`git svn find-rev $(git rev-list --date-order --max-count=1 $TRACKING_BRANCH)`

# Then do the diff from the highest revision on the current branch 
# and masssage into SVN format
git diff --no-prefix $(git rev-list --date-order --max-count=1 $TRACKING_BRANCH) $* |
sed -e "/--- \/dev\/null/{ N; s|^--- /dev/null\n+++ \(.*\)|--- \1	(revision 0)\n+++ \1	(revision 0)|;}" \
    -e "s/^--- .*/&	(revision $REV)/" \
    -e "s/^+++ .*/&	(working copy)/" \
    -e "s/^diff --git [^[:space:]]*/Index:/" \
    -e "s/^index.*/===================================================================/"

I did some modification on the original script from ( https://gist.github.com/dertom95/3551647 )
When run the original script, I get an error like: (I don't know how to fix it)

Code

$ git-svn-diff.sh >../a.diff
/bin/git-svn-diff.sh: line 17: conditional binary operator expected
/bin/git-svn-diff.sh: line 17: syntax error near `=~'
/bin/git-svn-diff.sh: line 17: `if [[ "$TRACKING_BRANCH" =~ URL.* ]]'

So, I just comment out some lines, since in my system, I only need the TRACKING_BRANCH value from the command:

Code

git config --get svn-remote.svn.fetch | sed -e 's/.*:refs\/remotes\///'

Which is "git-svn" in my local git repo.  

[ollydbg]

I commit the patch to trunk (r9848) now.