<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 12, 2015 at 6:05 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 class="">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>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></div></div></blockquote><div><br></div></span><div>This also happens when there's a missing brace at the end of your modular header, which is almost certainly the most common way to hit this problem in an already-modularized codebase. And it happens in codebases that use a mixture of modular and non-modular headers, where there's no reason to expect all the modular includes are before the non-modular ones.</div></div></div></div></blockquote><div><br></div><div>:lets-do-both.jpg:</div><div><br></div><div>Do we have a way to guard just notes behind warning flags? Maybe "-Winitial-module-map" or something could enable the "assume this module map might have errors" heuristics (which in my experience are highly reliable for that use case). We can document to users to turn this on when initially modularizing.<br></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"><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>125993 #include lines.<br></div><span><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></span></div><br></div></div>
</blockquote></div><br></div></div>