Author Topic: RFC remove the last argument LoaderBase* from Parser::Parse() function  (Read 4638 times)

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5226
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Request for comments for days.
I'm going to apply this patch:
Code: [Select]
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: [Select]
opts.loader = Manager::Get()->GetFileManager()->Load(filename, m_NeedsReparse);
and later the loader will be deleted in ParserThread::InitTokenizer() function.
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9496
I'm going to apply this patch:
As usual I cannot apply this GIT patch but from visual inspection it looks fine to me.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5226
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
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: [Select]
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: [Select]
#!/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: [Select]
$ 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: [Select]
git config --get svn-remote.svn.fetch | sed -e 's/.*:refs\/remotes\///'Which is "git-svn" in my local git repo.  ;)
« Last Edit: June 23, 2014, 11:26:45 am by ollydbg »
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5226
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
I commit the patch to trunk (r9848) now.
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.