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

[Richard Smith via cfe-commits]

On Wed, Aug 12, 2015 at 6:00 PM, Sean Silva <chisophugis at gmail.com> wrote:

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

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


> 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/3997cecc/attachment.html>