On Wed, Aug 12, 2015 at 6:00 PM, Sean Silva <chisophu...@gmail.com> wrote:
> > > 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. > Oops, apparently I accidentally hit send... I'm gathering some real statistics on this. -- Sean Silva > > >> >> 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