<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 12, 2015 at 6:00 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Wed, Aug 12, 2015 at 2:43 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On Wed, Aug 12, 2015 at 12:08 AM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">silvas added a subscriber: silvas.<br>
<span><br>
================<br>
Comment at: lib/Parse/Parser.cpp:2003<br>
@@ +2002,3 @@<br>
+ Diag(Tok, diag::err_unexpected_module_start);<br>
+ // Recover by skipping content of the included submodule.<br>
+ unsigned ModuleNesting = 1;<br>
----------------<br>
</span><span>rsmith wrote:<br>
> This is liable to produce bad follow-on diagnostics; I don't think this is a reasonable way to recover. I can see a few feasible options here:<br>
><br>
> 1) Emit a fatal error when this happens (suppressing further diagnostics)<br>
> 2) Break out to the top level of parsing and resume from there (that is, assume that a modular header expects to be included at the top level and that the user didn't screw up their module map)<br>
> 3) Enter the module and carry on parsing from here<br>
><br>
> My preference would be either (1) or (2). But either way, we should diagnose the still-open bracketing constructs, because the problem is likely to be a missing close brace at the end of an unrelated header file.<br>
</span>I strongly prefer (1). In all cases I have recorded in my notes when modularizing, the `error: expected '}'` diagnostic indicated one of two things:<br>
- that a header needed to be marked textual in the module map.<br>
- that a #include had to be moved to the top of the file (this case was likely behaving unexpectedly in the non-modules case and "happened to work").<br>
For the sake of our users, it is probably best to immediately fatal error and suggest an alternative (the suggestions can be a separate patch; my recommendations are below).<br>
<br>
I believe a user is most likely to run into this diagnostic when developing an initial set of module maps, so our diagnostic should be tailored to that audience.</blockquote><div><br></div></span><div>I think your observations may be biased by your current experiences modularizing existing code, and not representative of the complete development cycle. Modularization is a one-time effort; maintaining the code after modularization is a continuous process. I think it's *far* more likely that a header listed in your module map was expected to be modular, and that a brace mismatch within that file is unintentional and due to a missing brace somewhere.</div></div></div></div></blockquote><div><br></div></span><div>I don't think so. That implies that the inclusion is not at the top of the file, which is extremely unlikely in a modular codebase.</div><div><br></div><div>125993 #include lines.<br></div></div></div></div></blockquote><div><br></div><div>Oops, apparently I accidentally hit send...</div><div>I'm gathering some real statistics on this.</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>However, we should aim to provide diagnostics that are helpful for either case.</div><span><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">These users may have little experience with modules when they encounter this diagnostic, so a notes suggesting a likely remedy helps them develop confidence by providing authoritative advice (and avoiding a round trip to the documentation).<br>
<br>
My fine-grained recommendations from experience are:<br>
- #include inside extern "C": always meant to be modular, always the fix is to move it out of the braced block.<br>
- #include inside namespace: always meant to be modular, always the fix is to move it out of the braced block.<br>
- #include inside class: always textual (e.g. bringing in methods like TableGen .inc files; sometimes similar ".inc" files are manually written and not autogenerated. I have observed e.g. "classfoo_methods.h" files; another instance of this is stamping out ".def" files)<br>
- #include inside array initializer: always textual. Typically ".def" files<br>
- #include inside function: always textual. Typically ".def" files (e.g. generate a switch)<br>
<br>
".inl" files are theoretically a problem inside namespaces, but I have not observed this issue in practice. In my experience code taking the effort to have ".inl" files is quite clean and avoids such textual dependencies. The issues I have seen in practice with ".inl" files usually end up coming out through a different part of clang. Improving that is for a separate patch though.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D11844" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11844</a><br>
<br>
<br>
<br>
</blockquote></span></div><br></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>