r208345 - Remove -Wnon-modular-include
Alp Toker
alp at nuanti.com
Thu May 8 22:36:42 PDT 2014
On 09/05/2014 05:58, Alp Toker 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 :-)
>
> 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.
>
> 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.
Another great thing about doing remarks this way is that it'll solve the
-Weverything problem pointed out by Eli Friedman and Ted Kremenek which
Richard mentioned earlier in the thread, because it finally gives us a
useful -Reverything for free where sundry diagnostics will be displayed
"remark" rather than "warning".
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.
Alp.
>
>
> 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