On Wed, Aug 12, 2015 at 6:05 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
> 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. >> > > 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. > I think that handling the missing brace at end of module differently should be doable without disturbing the notes that Serge already has here. > 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. > We could check that the opening brace was in the file containing the import to avoid misdiagnosing a missing brace in a textual header. Richard, are these cases that you want Serge to deal with in the present patch? -- Sean Silva > > >> 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
