The second patch is more of a performance concern. Most projects have a Debug and Release target (where a DEBUG macro is defined for the Debug target). In this case when the target is changed, the user expects that the DEBUG code is enabled or disabled accordingly, which means the full project needs to be reparsed with the new macro state. Personally I don't mind the time taken for full project reparse, so I'd suggest committing the second patch too.
I even thought that we can switch the macro definition set when parsing different targets' source files, not sure this is doable. I do care about the performance issue. When developing C::B, I ways switch the build target from time to time. (I use thread search plugin, I need the search only limited to the current active target). So, I personally don't like the idea that switching the target cause a reparse. But I would like to see all the comments about this.
I understand about that.. I'm going to keep this feature aside for now, we'll see if we are able to find a better solution in the future.
Btw, I think you remember our fix for reliable UngetToken() behavior (
see here for the original discussion). To recap, in PeekToken(): we are checking the value of m_IsReplaceParsing before and after DoGetToken() to see if a ReplaceBufferText() was done, and if it was done, we should reset the savedTokenIndex, etc.
Now, this way of checking m_IsReplaceParsing can fail in some cases:
1) We are already in replace mode (eg, m_IsReplaceParsing == 1) before DoGetToken(), and inside DoGetToken() m_IsReplaceParsing is reset to zero and a new macro replacement is started (once again m_IsReplaceParsing == 1). No change will be detected and we will fail to reset the savedToken position.
2) A preprocessor condition is hit in SkipUnwanted() and we run CalcConditionExpression(). Now, CalcConditionExpression() will not only expand the macro, but also parse the entire expanded macro and compute the result. By the time we exit SkipUnwanted(), m_IsReplaceParsing is still non-zero, but we are already at the end of the expanded macro. Now, the DoGetToken() will reset m_IsReplaceParsing to zero, and once again no change will be detected. i.e.,
savedReplaceParsing = m_IsReplaceParsing; // m_IsReplaceParsing == 0
if (SkipUnwanted()) // ReplaceBufferText() was run and expanded macro parsed, m_IsReplaceParsing == 1
DoGetToken(); // out of replace buffer, m_IsReplaceParsing reset to 0
// check if ReplaceBufferText() was run
if (savedReplaceParsing < m_IsReplaceParsing) // condition failed
[...]
Yesterday I was unlucky enough to hit both these cases 1) and 2) in the same code, so we definitely need some improvement..
I think the proper way of doing it is to make the saved___ variables class members of Tokenizer, so they can be accessed from ReplaceBufferText() and reset when required (just like we are resetting the m_Undo___ variables). We also need to reset Undo and Saved positions to the proper place after CalcConditionExpression() (because as I've said before it's not only expanding the macro but also parsing it fully, so we need to reset right after the preprocessor line has ended).
I'll post the patch with these changes and some test cases below.