r206027 - Add -fmodules-strict-decluse to check that all headers are in modules
Richard Smith
richard at metafoo.co.uk
Wed Apr 16 13:42:57 PDT 2014
On Wed, Apr 16, 2014 at 12:24 PM, Daniel Jasper <djasper at google.com> wrote:
> On Wed, Apr 16, 2014 at 7:45 PM, Ben Langmuir <blangmuir at apple.com> wrote:
>
>> On Apr 16, 2014, at 10:28 AM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>>
>> On Wed, Apr 16, 2014 at 9:38 AM, Ben Langmuir <blangmuir at apple.com>wrote:
>>
>>>
>>> On Apr 14, 2014, at 5:47 PM, Richard Smith <richard at metafoo.co.uk>
>>> wrote:
>>>
>>> On Mon, Apr 14, 2014 at 11:53 AM, Ben Langmuir <blangmuir at apple.com>wrote:
>>>
>>>> To be clear, my patch does not involve decluse, so not quite the same.
>>>>
>>>
>>> There's no reason for this patch to be restricted to decluse (just as
>>> there's no reason for yours to be restricted to framework modules in the
>>> long term). The underlying intent is the same, and it doesn't make sense to
>>> have two different implementations of it.
>>>
>>>
>>> Are you suggesting folding these into one diagnostic and -W option? If
>>> so, is there a reasonable way to restrict it to frameworks on the command
>>> line, since that is an important restriction to us for now (unfortunately).
>>>
>>> Also, unlike decluse this is intended to be transitive (although I’m not
>>> sure that the below patch is *not* transitive… I just know that’s the
>>> intent for decluse). Is that okay?
>>>
>>
>> They're both intended to be transitive, AFAIK.
>>
>>
>> Interesting, that’s not what the comments
>> in test/Modules/Inputs/declare-use/module.map say. Transitive makes more
>> sense to me anyway.
>>
>
> decluse is currently explicitly not meant to be transitive. We only want
> to check whether the module we are currently building explicitly specifies
> its uses.
>
What does this mean if building one module causes us to build another
module that violates the decl-ref rules? (That is, when you're actually
building modules and not just using module maps for dependency checking?)
>
>
>> Here’s a strawman proposal:
>>
>> -Wnon-modular-include -- warn on any #include of a header that's not
>> listed in a module map
>> [-Wnon-modular-include-in-module -- subgroup, only warn if the include
>> is in a module]
>> -Wnon-modular-include-in-framework-module -- subgroup, only warn if
>> the include is in a framework module
>>
>> I don't think we have any demand for the middle option today, but I think
>> it's what you guys want eventually?
>>
>>
>> SGTM.
>>
>>
>> Ben
>>>
>>>
>>> Also (by way of review), I think this patch doesn’t handle umbrella
>>>> directories correctly, since it only looks for known headers.
>>>>
>>>
>>> Hmm, if we're getting that part wrong, we'll be getting it wrong for the
>>> decluse checking in general. Nice catch =)
>>>
>>>
>>>> On Apr 14, 2014, at 11:42 AM, Richard Smith <richard at metafoo.co.uk>
>>>> wrote:
>>>>
>>>> Why is this a -f option rather than a diagnostic? Why is it coupled to
>>>> -fmodules-decluse? See also Ben Langmuir's patch that does the same thing
>>>> for framework modules (also, oddly, with a language option).
>>>>
>>>>
>>>> On Fri, Apr 11, 2014 at 4:47 AM, Daniel Jasper <djasper at google.com>wrote:
>>>>
>>>>> Author: djasper
>>>>> Date: Fri Apr 11 06:47:45 2014
>>>>> New Revision: 206027
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=206027&view=rev
>>>>> Log:
>>>>> Add -fmodules-strict-decluse to check that all headers are in modules
>>>>>
>>>>> Review: http://reviews.llvm.org/D3335
>>>>>
>>>>> Added:
>>>>> cfe/trunk/test/Modules/strict-decluse.cpp
>>>>> Modified:
>>>>> cfe/trunk/include/clang/Basic/LangOptions.def
>>>>> cfe/trunk/include/clang/Driver/Options.td
>>>>> cfe/trunk/lib/Driver/Tools.cpp
>>>>> cfe/trunk/lib/Frontend/CompilerInvocation.cpp
>>>>> cfe/trunk/lib/Lex/ModuleMap.cpp
>>>>>
>>>>> Modified: cfe/trunk/include/clang/Basic/LangOptions.def
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.def?rev=206027&r1=206026&r2=206027&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- cfe/trunk/include/clang/Basic/LangOptions.def (original)
>>>>> +++ cfe/trunk/include/clang/Basic/LangOptions.def Fri Apr 11 06:47:45
>>>>> 2014
>>>>> @@ -96,6 +96,7 @@ LANGOPT(MathErrno , 1, 1, "errno
>>>>> BENIGN_LANGOPT(HeinousExtensions , 1, 0, "Extensions that we really
>>>>> don't like and may be ripped out at any time")
>>>>> LANGOPT(Modules , 1, 0, "modules extension to C")
>>>>> LANGOPT(ModulesDeclUse , 1, 0, "require declaration of module
>>>>> uses")
>>>>> +LANGOPT(ModulesStrictDeclUse, 1, 0, "require declaration of module
>>>>> uses and all headers to be in modules")
>>>>> LANGOPT(Optimize , 1, 0, "__OPTIMIZE__ predefined macro")
>>>>> LANGOPT(OptimizeSize , 1, 0, "__OPTIMIZE_SIZE__ predefined
>>>>> macro")
>>>>> LANGOPT(Static , 1, 0, "__STATIC__ predefined macro (as
>>>>> opposed to __DYNAMIC__)")
>>>>>
>>>>> Modified: cfe/trunk/include/clang/Driver/Options.td
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=206027&r1=206026&r2=206027&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- cfe/trunk/include/clang/Driver/Options.td (original)
>>>>> +++ cfe/trunk/include/clang/Driver/Options.td Fri Apr 11 06:47:45 2014
>>>>> @@ -607,6 +607,9 @@ def fmodules_ignore_macro : Joined<["-"]
>>>>> def fmodules_decluse : Flag <["-"], "fmodules-decluse">,
>>>>> Group<f_Group>,
>>>>> Flags<[DriverOption,CC1Option]>,
>>>>> HelpText<"Require declaration of modules used within a module">;
>>>>> +def fmodules_strict_decluse : Flag <["-"],
>>>>> "fmodules-strict-decluse">, Group<f_Group>,
>>>>> + Flags<[DriverOption,CC1Option]>,
>>>>> + HelpText<"Like -fmodules-decluse but requires all headers to be in
>>>>> modules">;
>>>>> def fretain_comments_from_system_headers : Flag<["-"],
>>>>> "fretain-comments-from-system-headers">, Group<f_Group>, Flags<[CC1Option]>;
>>>>>
>>>>> def fmudflapth : Flag<["-"], "fmudflapth">, Group<f_Group>;
>>>>> @@ -666,6 +669,8 @@ def fno_module_maps : Flag <["-"], "fno-
>>>>> Flags<[DriverOption]>;
>>>>> def fno_modules_decluse : Flag <["-"], "fno-modules-decluse">,
>>>>> Group<f_Group>,
>>>>> Flags<[DriverOption]>;
>>>>> +def fno_modules_strict_decluse : Flag <["-"],
>>>>> "fno-strict-modules-decluse">, Group<f_Group>,
>>>>> + Flags<[DriverOption]>;
>>>>> def fno_ms_extensions : Flag<["-"], "fno-ms-extensions">,
>>>>> Group<f_Group>;
>>>>> def fno_ms_compatibility : Flag<["-"], "fno-ms-compatibility">,
>>>>> Group<f_Group>;
>>>>> def fno_delayed_template_parsing : Flag<["-"],
>>>>> "fno-delayed-template-parsing">, Group<f_Group>;
>>>>>
>>>>> Modified: cfe/trunk/lib/Driver/Tools.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=206027&r1=206026&r2=206027&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- cfe/trunk/lib/Driver/Tools.cpp (original)
>>>>> +++ cfe/trunk/lib/Driver/Tools.cpp Fri Apr 11 06:47:45 2014
>>>>> @@ -3469,6 +3469,14 @@ void Clang::ConstructJob(Compilation &C,
>>>>> CmdArgs.push_back("-fmodules-decluse");
>>>>> }
>>>>>
>>>>> + // -fmodules-strict-decluse is like -fmodule-decluse, but also
>>>>> checks that
>>>>> + // all #included headers are part of modules.
>>>>> + if (Args.hasFlag(options::OPT_fmodules_strict_decluse,
>>>>> + options::OPT_fno_modules_strict_decluse,
>>>>> + false)) {
>>>>> + CmdArgs.push_back("-fmodules-strict-decluse");
>>>>> + }
>>>>> +
>>>>> // -fmodule-name specifies the module that is currently being built
>>>>> (or
>>>>> // used for header checking by -fmodule-maps).
>>>>> if (Arg *A = Args.getLastArg(options::OPT_fmodule_name)) {
>>>>>
>>>>> Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=206027&r1=206026&r2=206027&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
>>>>> +++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Fri Apr 11 06:47:45
>>>>> 2014
>>>>> @@ -1343,7 +1343,9 @@ static void ParseLangArgs(LangOptions &O
>>>>> Opts.Blocks = Args.hasArg(OPT_fblocks);
>>>>> Opts.BlocksRuntimeOptional =
>>>>> Args.hasArg(OPT_fblocks_runtime_optional);
>>>>> Opts.Modules = Args.hasArg(OPT_fmodules);
>>>>> - Opts.ModulesDeclUse = Args.hasArg(OPT_fmodules_decluse);
>>>>> + Opts.ModulesStrictDeclUse =
>>>>> Args.hasArg(OPT_fmodules_strict_decluse);
>>>>> + Opts.ModulesDeclUse =
>>>>> + Args.hasArg(OPT_fmodules_decluse) || Opts.ModulesStrictDeclUse;
>>>>> Opts.CharIsSigned = Opts.OpenCL ||
>>>>> !Args.hasArg(OPT_fno_signed_char);
>>>>> Opts.WChar = Opts.CPlusPlus && !Args.hasArg(OPT_fno_wchar);
>>>>> Opts.ShortWChar = Args.hasFlag(OPT_fshort_wchar,
>>>>> OPT_fno_short_wchar, false);
>>>>>
>>>>> Modified: cfe/trunk/lib/Lex/ModuleMap.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/ModuleMap.cpp?rev=206027&r1=206026&r2=206027&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- cfe/trunk/lib/Lex/ModuleMap.cpp (original)
>>>>> +++ cfe/trunk/lib/Lex/ModuleMap.cpp Fri Apr 11 06:47:45 2014
>>>>> @@ -243,8 +243,12 @@ void ModuleMap::diagnoseHeaderInclusion(
>>>>> resolveUses(RequestingModule, /*Complain=*/false);
>>>>>
>>>>> HeadersMap::iterator Known = findKnownHeader(File);
>>>>> - if (Known == Headers.end())
>>>>> + if (Known == Headers.end()) {
>>>>> + if (LangOpts.ModulesStrictDeclUse)
>>>>> + Diags.Report(FilenameLoc, diag::error_undeclared_use_of_module)
>>>>> + << RequestingModule->getFullModuleName() << Filename;
>>>>> return;
>>>>> + }
>>>>>
>>>>> Module *Private = NULL;
>>>>> Module *NotUsed = NULL;
>>>>>
>>>>> Added: cfe/trunk/test/Modules/strict-decluse.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/strict-decluse.cpp?rev=206027&view=auto
>>>>>
>>>>> ==============================================================================
>>>>> --- cfe/trunk/test/Modules/strict-decluse.cpp (added)
>>>>> +++ cfe/trunk/test/Modules/strict-decluse.cpp Fri Apr 11 06:47:45 2014
>>>>> @@ -0,0 +1,9 @@
>>>>> +// RUN: rm -rf %t
>>>>> +// RUN: %clang_cc1 -fmodule-maps -fmodules-cache-path=%t
>>>>> -fmodules-strict-decluse -fmodule-name=XG -I %S/Inputs/declare-use %s
>>>>> -verify
>>>>> +
>>>>> +#include "g.h"
>>>>> +#include "e.h"
>>>>> +#include "f.h" // expected-error {{module XG does not depend on a
>>>>> module exporting 'f.h'}}
>>>>> +#include "i.h" // expected-error {{module XG does not depend on a
>>>>> module exporting 'i.h'}}
>>>>> +
>>>>> +const int g2 = g1 + e + f + aux_i;
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140416/a5aaec76/attachment.html>
More information about the cfe-commits
mailing list