r208345 - Remove -Wnon-modular-include

[Manuel Klimek]

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.

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