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. 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
