r208345 - Remove -Wnon-modular-include

Argyrios Kyrtzidis akyrtzi at gmail.com
Thu May 8 23:34:59 PDT 2014


On May 8, 2014, at 9:58 PM, Alp Toker <alp at nuanti.com> wrote:

> 
> On 09/05/2014 04:43, Argyrios Kyrtzidis wrote:
>> On May 8, 2014, at 5:31 PM, Alp Toker <alp at nuanti.com> wrote:
>> 
>>> On 09/05/2014 01:08, Argyrios Kyrtzidis wrote:
>>>> On May 8, 2014, at 4:53 PM, Richard Smith <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
>>>> 
>>>>> On Thu, May 8, 2014 at 4:39 PM, Richard Smith <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
>>>>> 
>>>>>    On Thu, May 8, 2014 at 4:28 PM, Argyrios Kyrtzidis
>>>>>    <akyrtzi at gmail.com <mailto:akyrtzi at gmail.com>> wrote:
>>>>> 
>>>>> 
>>>>>        On May 8, 2014, at 4:23 PM, Richard Smith
>>>>>        <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
>>>>> 
>>>>>>        On Thu, May 8, 2014 at 4:21 PM, Argyrios
>>>>>>        Kyrtzidis<akyrtzi at gmail.com <mailto:akyrtzi at gmail.com>>wrote:
>>>>>> 
>>>>>>            Hi Richard,
>>>>>> 
>>>>>>            On May 8, 2014, at 1:27 PM, Richard Smith
>>>>>>            <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>>
>>>>>>            wrote:
>>>>>> 
>>>>>>>            On Thu, May 8, 2014 at 1:20 PM, Richard
>>>>>>>            Smith<richard at metafoo.co.uk
>>>>>>>            <mailto:richard at metafoo.co.uk>>wrote:
>>>>>>> 
>>>>>>>                On Thu, May 8, 2014 at 11:09 AM, Ben
>>>>>>>                Langmuir<blangmuir at apple.com
>>>>>>>                <mailto:blangmuir at apple.com>>wrote:
>>>>>>> 
>>>>>>>                    Author: benlangmuir
>>>>>>>                    Date: Thu May 8 13:09:29 2014
>>>>>>>                    New Revision: 208345
>>>>>>> 
>>>>>>>                    URL:http://llvm.org/viewvc/llvm-project?rev=208345&view=rev
>>>>>>>                    Log:
>>>>>>>                    Remove -Wnon-modular-include
>>>>>>> 
>>>>>>>                    But keep
>>>>>>>                    -Wnon-modular-include-in-[framework-]module
>>>>>>> 
>>>>>>>                    This warning is too noisy and doesn't really
>>>>>>>                    indicate a problem for most
>>>>>>>                    people. Even though it would only really affect
>>>>>>>                    people using
>>>>>>>                    -Weverything, that seems bad so remove it.
>>>>>>> 
>>>>>>> 
>>>>>>>                Wait a second, we were planning on using this to
>>>>>>>                replace -fmodules-strict-decluse. People using
>>>>>>>                -Weverything need to use -Wno-foo for the warnings
>>>>>>>                they don't want; that's the cost of using that option.
>>>>>>> 
>>>>>>> 
>>>>>>>            From when -Weverything was added:
>>>>>>> 
>>>>>>>            Eli Friedman wrote:
>>>>>>>            > This seems like a bad idea: people will start using
>>>>>>>            it, then complain
>>>>>>>            > whenever we add a new warning which isn't generically
>>>>>>>            applicable
>>>>>>>            > (suppose we add an opt-in warning for C-style casts
>>>>>>>            in C++, for
>>>>>>>            > example). We already have -Wextra for people who want
>>>>>>>            lots of> warnings.
>>>>>>> 
>>>>>>>            Ted Kremenek responded:
>>>>>>>            [...]
>>>>>>>            > -Weverything is not for everybody, and for those that
>>>>>>>            complain about
>>>>>>>            > new warnings we can (and should) tell them to take
>>>>>>>            the opt-in approach
>>>>>>>            > as opposed to the opt-out approach. I don't see a
>>>>>>>            problem with really
>>>>>>>            > serving both sets of user preferences.
>>>>>>> 
>>>>>>>            => We do not get to use -Weverything as an excuse for
>>>>>>>            removing warning that are noisy and that some users
>>>>>>>            aren't interested in.
>>>>>>            This is not just noisy, it has no value IMO. How can we
>>>>>>            expect an application not to #include something
>>>>>>            textually and what is the user supposed to do when they
>>>>>>            see this warning ?
>>>>>> 
>>>>>> 
>>>>>>        If the application intends to have all of its own headers
>>>>>>        modularized, this warning can be used to check that. If the
>>>>>>        user sees this warning, and they're in that case, they
>>>>>>        should either add the header to a module or explicitly
>>>>>>        exclude it (if it's supposed to be textually included).
>>>>>        I don’t think this is feasible in practice, preprocessor
>>>>>        tricks and textual includes will not go away, they have real
>>>>>        uses.
>>>>> 
>>>>> 
>>>>>    They do, but you can exclude headers meant for textual inclusion
>>>>>    just as easily for an application as for a library.
>>>>> 
>>>>>        The warning is very important for the framework/library
>>>>>        author, but I don’t think it provides value in practice for
>>>>>        the application author.
>>>>> 
>>>>> 
>>>>>    I disagree. An application may want to use modules for itself.
>>>>>    They're not limited to libraries.
>>>>> 
>>>>> 
>>>>> I've done some more digging and I think what we want to replace -fmodules-strict-decluse is to warn if either
>>>>> a) a module build includes a header that's not referenced by any module map, or
>>>>> b) a non-module build with -fmodule-name= specified references such a header
>>>>> ... which looks like it matches the cases that the remaining -Wnon-modular-include-in-module catches, so that's a non-issue.
>>>>> 
>>>>> However, for my work modularizing LLVM, the -Wnon-modular-include warning has *already* been very useful, despite having only been available for a few hours. I have first-hand experience that this *is* useful functionality for application authors. That said, I think we should suppress the warning if the includer isn't in the main source file (I don't care that my non-modular includes have transitive non-modular includes.)
>>>> How about adding it as a remark ? That way you can get the same benefit as before without the negative connotation that the user is doing something “wrong” by simply #including something.
>>> I'd be cautious of categorising this at *lower* severity than, say, -Wold-style-cast, because the distinction appears arbitrary. Are C-style casts more severe than non-modular includes? How long is a piece of string?
>> FWIW, I'm not fond of -Wold-style-cast. I subscribe to the notion that compiler warnings should point out potential problems in the code, not enforce style; style should be the domain of another tool.
> 
> Are mismatched HTML tags and empty paragraphs in comments really problems in the code? Someone thought so and now we have several such warnings in clang. Others would say that comments aren't even code :-)

Yes, mismatched HTML tags (and related documentation warnings) is a problem, it's not a "style". When you opt-in for documentation comment parsing and warnings you opt-in to treat documentation comments on the same high standard as your code.

> 
> Meanwhile the communities who requested the -Wold-style-cast feature consider it a strict error, so much so that they'd disable* it entirely if they could.
> 
> As a GCC-compatible frontend we have dozens of such warnings which seem silly to any well-adjusted person, yet have plenty of devotees.
> 
> This all goes to show that warning severities are subjective. Personally I still don't see how non-modular-include could be considered less severe than warnings about whitespace, comments or coding style that we already have.

The argument that we have other warnings that don't actually point to problems is not convincing to me that we should add more of those.

> 
> For that reason it's not clear how the built-in diagnostic level patch landed a few weeks ago can ever work out -- and nobody's proven otherwise yet.
> 
> Fortunately we already have a "remark" level that's actually viable and has workable semantics -- it's called DefaultIgnore.

Hmm, there is a difference between a warning that is designed to find problems but ends up too noisy in practice and a "remark", we should not conflate the two concepts.

> 
> Alp.
> 
> 
>> 
>>> The existing set of off-by-default warnings would seem fine for this, or perhaps clang-tidy.
>>> 
>>> Alp.
>>> 
>>> -- 
>>> http://www.nuanti.com
>>> the browser experts
>>> 
> 
> -- 
> http://www.nuanti.com
> the browser experts
> 





More information about the cfe-commits mailing list