249999 - Eliminate Field |fTokenizerAdvanced| from |nsIMAPGenericParser|

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Hans-Andreas Engel]

The field |fTokenizerAdvanced| is used in |nsIMAPGenericParser::GetNextToken| to
determine if |fCurrentTokenPlaceHolder| should be reset to |fLineOfTokens". 
However, when this reset occurs, it never modifies |fCurrentTokenPlaceHolder|. 
Therefore, we can remove the Field |fTokenizerAdvanced| to make the imap code
easier to read.

Comment 1

[Hans-Andreas Engel]

Below, I would like to explain why the field |PRBool
nsIMAPGenericParser::fTokenizerAdvanced| can be eliminated from the imap code.

We observe that |fTokenizerAdvanced| is used to determine whether
|fLineOfTokens| or |fCurrentTokenPlaceHolder| is used as starting point for
parsing the next token in |GetNextToken()|.

This effect of setting |fTokenizerAdvanced| is illuminated by identifying the
cases when |fCurrentTokenPlaceHolder| is modified but these changes are
subsequently ignored.  The changes are ignored and |fCurrentTokenPlaceHolder| is
reset
  i)   in |GetNextToken()| if fTokenizerAdvanced==TRUE,
  ii)  in |GetNextToken()| if fAtEndOfLine==TRUE,
  iii) in |AdvanceToNextLine()|, and
  iv)  in |AdvanceTokenizerStartingPoint()|.

Note that |fTokenizerAdvanced| is always reset in |GetNextToken| or
|AdvanceToNextLine|, so we do not need to worry about case i) after cases ii) or
iii).

Let us now consider the places where |fCurrentTokenPlaceHolder| is modified.

a) |nsIMAPGenericParser::CreateLiteral()|: Note that the modification of
|fCurrentTokenPlaceHolder| is anyway thrown away later, due to iv) or ii) [the
latter occurs since |fAtEndOfLine=PR_TRUE| is set and |GetNextToken| or
|AdvanceToNextLine| are always invoked subsequent to |CreateLiteral|].
  Further, the second |fCurrentTokenPlaceHolder++| in
|nsIMAPGenericParser::CreateLiteral()| is never executed, because
|AdvanceTokenizerStartingPoint| is previously called and all token boundaries
are recovered, so it is impossible to 'land on a token boundary.' (see bug 248933)
  Since the modifications are thrown away anyway, case i) is not relevant here.

b) |nsIMAPGenericParser::CreateParenGroup()|: The modifications of
|fCurrentTokenPlaceHolder| are never thrown away.  |fTokenizerAdvanced=FALSE| is
set explicitly to avoid case i).  After some direct access to
|fCurrentTokenPlaceHolder|, |GetNextToken| is always called and
|fCurrentTokenPlaceHolder| is reset.

c) |nsIMAPGenericParser::CreateQuoted|:  This method is can be called from b),
see discussion there.  Additionally, it can be called between two invocations of
|GetNextToken()|, which sets fTokenizerAdvanced=PR_FALSE and thus case i) never
applies.  Finally, |CreateQuoted| is called via |CreateNilString| or
|CreateAstring|.  Checking these calls in nsImapServerResponseParser.cpp and
nsIMAPBodyShell.cpp reveals that they are always preceded by |GetNextToken()|
and followed by either |GetNextToken()|, |at_end_of_line()| occurs, or
|nsImapServerResponseParser::end_of_line(){ ... ResetLexAnalyzer() ... }|, and
thus i) never happens.

In summary, case i) is never relevant.  This means that we can eliminate the
check for |fTokenizerAdvanced| in |GetNextToken()|, without changing any
functionality of the code.

Note that |nsImapServerResponseParser::mailbox| uses |fTokenizerAdvanced|.  
However, this is only a kludge to avoid Bug 248933 will be removed by the patch
for Bug 248933.

Finally, we see that |fTokenizerAdvanced| has no effect on the imap code and can
be completely eliminated!

Comment 2

[Hans-Andreas Engel]

As a heuristic test, I insert the assertion
  assert(!fTokenizerAdvanced || fLineOfTokens==fCurrentTokenPlaceHolder);
in |GetNextToken|, just before |if (fTokenizerAdvanced)|.

Testing on my imap account, this assertion always holds, confirming the argument
above.

Comment 3

[Hans-Andreas Engel]

Attachment: Remove Field fTokenizerAdvanced from nsIMAPGenericParser

Requires that Bug 248933 is fixed first

Comment 4

[Hans-Andreas Engel]

Attachment: Remove Field fTokenizerAdvanced from nsIMAPGenericParser, works with current trunk (Jan 2005)

Since the previous patch awaits review for more than six months, it has become
somewhat rotten.

The updated patch implements the same changes, but works with the current
trunk.	Please review either patch soon.

Comment 5

[David :Bienvenu]

fixed, Thanks, Hans-Andreas

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using code inspection on http://lxr.mozilla.org