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

[Sean Silva via cfe-commits]

On Wed, Aug 12, 2015 at 2:43 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

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

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
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150812/0e0bea5d/attachment.html>