<div dir="ltr">I'm not especially fond of treating annot_module_begin and annot_module_include differently. That'll mean we get completely different diagnostics for a '#include' in a bad place depending on whether it's including something in the same top-level module or something in a different top-level module.<div>
<br></div><div>I'm also not sure that the diagnostic change here is an improvement. Previously:</div><div><br></div><div>#include "not_module.h"</div><div>#include "module.h"<br></div><div><br></div>
<div>... where not_module.h contains:</div><div><br></div><div>class X {</div><div> // ...</div><div>// whoops, forgot the '};'</div><div><br></div><div>would give a "missing '}'" diagnostic, pointing at the '{' that is left un-closed. It now gives a mysterious 'must be at the top level' diagnostic, not pointing at the '}' any more, followed by more errors (because 'module.h' didn't get imported and because the code after the #include is still within the class). The error here was almost certainly that a '}' was missing, so this seems like a regression.</div>
<div><br></div><div>In what cases does this change allow us to recover from errors better?</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Feb 5, 2014 at 11:10 AM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
On Feb 5, 2014, at 10:48 AM, Ben Langmuir <<a href="mailto:blangmuir@apple.com">blangmuir@apple.com</a>> wrote:<br>
<br>
> Hi again,<br>
><br>
> This patch grew a bit to handle a bunch of other non-top-level contexts that imports might show up in. Previously annot_module_include signaled an end-of-module, which the parser used to stop parsing in a bunch of places, and then you would get unfortunate diagnostics like this when the parser broke out of whatever context it was in<br>
><br>
> void foo() {<br>
> #include “bar.h” // error missing }<br>
> } // error extraneous closing brace (})<br>
><br>
><br>
> I wasn’t sure how to say ‘must be at top level’ in the diagnostic. I only found one existing diagnostic that mentions ‘top level’, but several that refer to ‘file scope’, so I went with the latter. I’m happy to change that if there is a convention I didn’t pick up on.<br>
<br>
</div></div>I think inside a namespace is also “file scope” so I prefer “top level scope” here.<br>
<br>
Some other nitpicks:<br>
<br>
-Watch out for 80 columns violations.<br>
<br>
-I’d prefer that we reduce the amount of passing around of the "llvm::PointerIntPair<Module *, 2, tok::IncludeDirectiveKind> object", e.g. maybe have a<br>
getModuleImportAnnotationInfo<br>
method in Token which will return a std::pair of (Module *, DirectiveKind) or a wrapper struct of such info, or return the Module* directly and the DirectiveKind as an optional out parameter.<br>
<br>
<br>
><br>
> Ben<br>
><br>
> <import.patch><br>
<div class="HOEnZb"><div class="h5">><br>
> On Feb 4, 2014, at 8:48 AM, Douglas Gregor <<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>> wrote:<br>
><br>
>><br>
>> On Feb 4, 2014, at 8:19 AM, Ben Langmuir <<a href="mailto:blangmuir@apple.com">blangmuir@apple.com</a>> wrote:<br>
>><br>
>>><br>
>>> On Feb 3, 2014, at 4:51 PM, Douglas Gregor <<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>> wrote:<br>
>>><br>
>>>><br>
>>>> On Feb 3, 2014, at 2:32 PM, Ben Langmuir <<a href="mailto:blangmuir@apple.com">blangmuir@apple.com</a>> wrote:<br>
>>>><br>
>>>>> Based on a suggestion from Jordan I’ve dropped the extra note, which will be on the same location anyway, and added that information into the error diagnostic. I would have liked to say "treating #include ...” instead of "treating directive …", but after the preprocessor/lexer this information is lost and I don’t see a nice way to pass it on to the parser.<br>
>>>><br>
>>>> There’s an ugly way to pass it on to the parser… you have a couple low bits in the token’s annotation value to record #include vs. #import vs. #include_next.<br>
>>><br>
>>> To be clear, are you suggesting making Token::PtrData a PointerIntPair?<br>
>><br>
>> Token::PtrData is an opaque pointer with different meanings for different annotation token kinds. For a module import token, that could be PointerIntPair<Module *, 2, IncludeDirectiveKind>.<br>
>><br>
>> - Doug<br>
>><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
</div></div></blockquote></div><br></div>