On Wed, Aug 12, 2015 at 2:43 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
> On Wed, Aug 12, 2015 at 12:08 AM, Sean Silva <chisophu...@gmail.com> > wrote: > >> silvas added a subscriber: silvas. >> >> ================ >> Comment at: lib/Parse/Parser.cpp:2003 >> @@ +2002,3 @@ >> + Diag(Tok, diag::err_unexpected_module_start); >> + // Recover by skipping content of the included submodule. >> + unsigned ModuleNesting = 1; >> ---------------- >> rsmith wrote: >> > 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: >> > >> > 1) Emit a fatal error when this happens (suppressing further >> diagnostics) >> > 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) >> > 3) Enter the module and carry on parsing from here >> > >> > 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. >> I strongly prefer (1). In all cases I have recorded in my notes when >> modularizing, the `error: expected '}'` diagnostic indicated one of two >> things: >> - that a header needed to be marked textual in the module map. >> - 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"). >> 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). >> >> 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. > > > 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. > 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. 125993 #include lines. > > However, we should aim to provide diagnostics that are helpful for either > case. > > 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). >> >> My fine-grained recommendations from experience are: >> - #include inside extern "C": always meant to be modular, always the fix >> is to move it out of the braced block. >> - #include inside namespace: always meant to be modular, always the fix >> is to move it out of the braced block. >> - #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) >> - #include inside array initializer: always textual. Typically ".def" >> files >> - #include inside function: always textual. Typically ".def" files (e.g. >> generate a switch) >> >> ".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. >> >> >> http://reviews.llvm.org/D11844 >> >> >> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
