r208345 - Remove -Wnon-modular-include

Fri May 9 09:35:08 PDT 2014

On May 9, 2014, at 3:01 AM, Manuel Klimek <klimek at google.com> wrote:

> On Fri, May 9, 2014 at 2:08 AM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> 
> On May 8, 2014, at 4:53 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 
>> On Thu, May 8, 2014 at 4:39 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> On Thu, May 8, 2014 at 4:28 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>> 
>> On May 8, 2014, at 4:23 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> 
>>> On Thu, May 8, 2014 at 4:21 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>>> Hi Richard,
>>> 
>>> On May 8, 2014, at 1:27 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>> 
>>>> On Thu, May 8, 2014 at 1:20 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>>> On Thu, May 8, 2014 at 11:09 AM, Ben Langmuir <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.
> 
> -fmodules-strict-decluse is definitely designed to find problems in the code - it's there to help a code base enforce (module-like) layering, even when using textual inclusions. Layering is in my opinion not a style issue, but a code quality issue.

A separate flag is perfectly fine, I'm mainly objecting to adding an unconditional warning that hits in any textual include.

> 
> Cheers,
> /Manuel
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140509/9c50b5c7/attachment.html>