[cfe-commits] [Patch 1 of 4] -verify fixes (stage 2)

[Douglas Gregor]

On Jul 28, 2012, at 12:07 AM, Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:

> On Saturday, July 28, 2012 3:18 AM, Jordan Rose wrote:
>> On Jul 26, 2012, at 2:16 AM, Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:
>>> I have implemented all of your comments, except the one relating to the
>>> tracking on Begin/EndSourceFile calls, for three reasons: firstly, I need
>>> to track in order to correctly attach and detach the preprocessor hooks;
>>> secondly, doing so highlighted the mismatch bug and (who knows) may do
>>> again in future; thirdly, when it comes the ARCMT tests fix in patch 2,
>>> I rely on knowing when the last EndSourceFile has been called.
>> 
>> Looks pretty good, but there's one last thing I'm not sure about. It kind
>> of scares me that BeginSourceFile/EndSourceFile calls can be nested; I
>> always thought that they were more BeginTranslationUnit /
>> EndTranslationUnit, since they are most definitely NOT called for
>> #includes. But they do seem to be called for searching module map files
>> and for "chained includes" (which, by the way, seems like it could also
>> have an unbalanced begin/end pair on error).
>> 
>> If they really are supposed to be nested, then your code is correct. If
>> not, though, we should probably just be using a boolean for this.
> 
> I can't speak for the original implementors and what their intention was
> for BeginSourceFile/EndSourceFile but from my observations, it seems they
> are called for each source file which is "outside" the main source file.
> This means, as you say, it is not called for #includes since these are
> "within" the main source file (i.e. incorporated by the preprocessor),
> but is called for module map files and also the -ast-merge function.  I
> hadn't observed it with chained includes and I will go and check whether
> there is a mismatched begin/end pair there.


IIRC, they are called for module map files and -ast-merge'd files because the language options might be different, and because we're potentially using a different source manager for diagnostics when we're (e.g.) separately compiling a module. I'm completely up for revisiting this contract, however; -ast-merge is simply a testing tool, and can be changed on a whim, and modules are still very much in flux.

	- Doug