[PATCH] D11844: [Modules] More descriptive diagnostics for misplaced import directive

[Richard Smith via cfe-commits]

On Wed, Aug 12, 2015 at 12:08 AM, Sean Silva <chisophugis at 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150812/cfc5db1f/attachment.html>