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

Sean Silva via cfe-commits cfe-commits at lists.llvm.org
Mon Aug 17 19:34:58 PDT 2015


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

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

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


More information about the cfe-commits mailing list