r208345 - Remove -Wnon-modular-include

[Alp Toker]

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?

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