<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Apr 17, 2014 at 7:02 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="">On Thu, Apr 17, 2014 at 3:28 AM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>On Wed, Apr 16, 2014 at 10:42 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>On Wed, Apr 16, 2014 at 12:24 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>On Wed, Apr 16, 2014 at 7:45 PM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div>On Apr 16, 2014, at 10:28 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div>
<blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Apr 16, 2014 at 9:38 AM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div>On Apr 14, 2014, at 5:47 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Apr 14, 2014 at 11:53 AM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">To be clear, my patch does not involve decluse, so not quite the same.</div></blockquote>
<div><br></div><div>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.</div>
</div></div></div></blockquote><div><br></div></div><div>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).</div>
<div><br></div><div>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?</div></div></div></blockquote>
<div><br></div><div>They're both intended to be transitive, AFAIK.</div></div></div></div></blockquote><div><br></div></div><div>Interesting, that’s not what the comments in test/Modules/Inputs/declare-use/module.map say. Transitive makes more sense to me anyway.</div>
</div></div></blockquote><div><br></div></div><div>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.</div></div>
</div></div></blockquote><div><br></div></div><div>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?)</div>
</div></div></div></blockquote><div><br></div></div><div>Well, then we should issue a warning, when we are building that module transitively. But if say, we are building module A and that requires module B and then B has previously been built ignoring the warning. Then I think we should not complain about that missing usage again.</div>
</div></div></div></blockquote><div><br></div></div><div>That's what we *will* do, but I don't think it's what we *should* do: the warnings we produce for a build really shouldn't depend on what happens to be in the module cache when the build begins. (Ultimately, this is one of the weaknesses making the module build step implicit, and eventually I think we'll want to give people an easy way to make it explicit.)</div>
</div></div></div></blockquote><div><br></div><div>1. I assumed when using a build system the modules "cache" would basically always be prepopulated with the result of the compilation of the dependencies (very much like Java builds do)</div>
<div>2. I strongly disagree: I think the warning of a module being compiled should always be the same, independently whether its compile happens to be triggered from a different module compilation; note that afaiu this is the way it happens to work in other languages that have similar "auto-build" triggering logic (I cross-checked how Java does it); if clang wants to go down a different path, I'd expect to have a very compelling argument to do so.</div>
<div><br></div><div>Cheers,</div><div>/Manuel</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>Nonetheless, it seems that we can still use exactly the same model for your warning and Ben's: warn on any direct inclusions into the current module (and not in inclusions into things that are included into that module). If/when we build the dependency modules, we'll warn on their direct inclusions, and so on.</div>
<div><div class="h5">
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Here’s a strawman proposal:</div><div><br></div><div>-Wnon-modular-include -- warn on any #include of a header that's not listed in a module map</div>
<div> [-Wnon-modular-include-in-module -- subgroup, only warn if the include is in a module]</div><div> -Wnon-modular-include-in-framework-module -- subgroup, only warn if the include is in a framework module</div><div>
<br></div><div>I don't think we have any demand for the middle option today, but I think it's what you guys want eventually?</div></div></div></div></blockquote><div><br></div></div><div>SGTM.</div><div><div>
<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><span><font color="#888888">Ben</font></span><div><div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Also (by way of review), I think this patch doesn’t handle umbrella directories correctly, since it only looks for known headers.</div>
</blockquote><div><br></div><div>Hmm, if we're getting that part wrong, we'll be getting it wrong for the decluse checking in general. Nice catch =)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div>On Apr 14, 2014, at 11:42 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div><br><blockquote type="cite">
<div dir="ltr">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).</div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Apr 11, 2014 at 4:47 AM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: djasper<br>
Date: Fri Apr 11 06:47:45 2014<br>
New Revision: 206027<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=206027&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=206027&view=rev</a><br>
Log:<br>
Add -fmodules-strict-decluse to check that all headers are in modules<br>
<br>
Review: <a href="http://reviews.llvm.org/D3335" target="_blank">http://reviews.llvm.org/D3335</a><br>
<br>
Added:<br>
cfe/trunk/test/Modules/strict-decluse.cpp<br>
Modified:<br>
cfe/trunk/include/clang/Basic/LangOptions.def<br>
cfe/trunk/include/clang/Driver/Options.td<br>
cfe/trunk/lib/Driver/Tools.cpp<br>
cfe/trunk/lib/Frontend/CompilerInvocation.cpp<br>
cfe/trunk/lib/Lex/ModuleMap.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/LangOptions.def<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.def?rev=206027&r1=206026&r2=206027&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.def?rev=206027&r1=206026&r2=206027&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/LangOptions.def (original)<br>
+++ cfe/trunk/include/clang/Basic/LangOptions.def Fri Apr 11 06:47:45 2014<br>
@@ -96,6 +96,7 @@ LANGOPT(MathErrno , 1, 1, "errno<br>
BENIGN_LANGOPT(HeinousExtensions , 1, 0, "Extensions that we really don't like and may be ripped out at any time")<br>
LANGOPT(Modules , 1, 0, "modules extension to C")<br>
LANGOPT(ModulesDeclUse , 1, 0, "require declaration of module uses")<br>
+LANGOPT(ModulesStrictDeclUse, 1, 0, "require declaration of module uses and all headers to be in modules")<br>
LANGOPT(Optimize , 1, 0, "__OPTIMIZE__ predefined macro")<br>
LANGOPT(OptimizeSize , 1, 0, "__OPTIMIZE_SIZE__ predefined macro")<br>
LANGOPT(Static , 1, 0, "__STATIC__ predefined macro (as opposed to __DYNAMIC__)")<br>
<br>
Modified: cfe/trunk/include/clang/Driver/Options.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=206027&r1=206026&r2=206027&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=206027&r1=206026&r2=206027&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Driver/Options.td (original)<br>
+++ cfe/trunk/include/clang/Driver/Options.td Fri Apr 11 06:47:45 2014<br>
@@ -607,6 +607,9 @@ def fmodules_ignore_macro : Joined<["-"]<br>
def fmodules_decluse : Flag <["-"], "fmodules-decluse">, Group<f_Group>,<br>
Flags<[DriverOption,CC1Option]>,<br>
HelpText<"Require declaration of modules used within a module">;<br>
+def fmodules_strict_decluse : Flag <["-"], "fmodules-strict-decluse">, Group<f_Group>,<br>
+ Flags<[DriverOption,CC1Option]>,<br>
+ HelpText<"Like -fmodules-decluse but requires all headers to be in modules">;<br>
def fretain_comments_from_system_headers : Flag<["-"], "fretain-comments-from-system-headers">, Group<f_Group>, Flags<[CC1Option]>;<br>
<br>
def fmudflapth : Flag<["-"], "fmudflapth">, Group<f_Group>;<br>
@@ -666,6 +669,8 @@ def fno_module_maps : Flag <["-"], "fno-<br>
Flags<[DriverOption]>;<br>
def fno_modules_decluse : Flag <["-"], "fno-modules-decluse">, Group<f_Group>,<br>
Flags<[DriverOption]>;<br>
+def fno_modules_strict_decluse : Flag <["-"], "fno-strict-modules-decluse">, Group<f_Group>,<br>
+ Flags<[DriverOption]>;<br>
def fno_ms_extensions : Flag<["-"], "fno-ms-extensions">, Group<f_Group>;<br>
def fno_ms_compatibility : Flag<["-"], "fno-ms-compatibility">, Group<f_Group>;<br>
def fno_delayed_template_parsing : Flag<["-"], "fno-delayed-template-parsing">, Group<f_Group>;<br>
<br>
Modified: cfe/trunk/lib/Driver/Tools.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=206027&r1=206026&r2=206027&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=206027&r1=206026&r2=206027&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Driver/Tools.cpp (original)<br>
+++ cfe/trunk/lib/Driver/Tools.cpp Fri Apr 11 06:47:45 2014<br>
@@ -3469,6 +3469,14 @@ void Clang::ConstructJob(Compilation &C,<br>
CmdArgs.push_back("-fmodules-decluse");<br>
}<br>
<br>
+ // -fmodules-strict-decluse is like -fmodule-decluse, but also checks that<br>
+ // all #included headers are part of modules.<br>
+ if (Args.hasFlag(options::OPT_fmodules_strict_decluse,<br>
+ options::OPT_fno_modules_strict_decluse,<br>
+ false)) {<br>
+ CmdArgs.push_back("-fmodules-strict-decluse");<br>
+ }<br>
+<br>
// -fmodule-name specifies the module that is currently being built (or<br>
// used for header checking by -fmodule-maps).<br>
if (Arg *A = Args.getLastArg(options::OPT_fmodule_name)) {<br>
<br>
Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=206027&r1=206026&r2=206027&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=206027&r1=206026&r2=206027&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)<br>
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Fri Apr 11 06:47:45 2014<br>
@@ -1343,7 +1343,9 @@ static void ParseLangArgs(LangOptions &O<br>
Opts.Blocks = Args.hasArg(OPT_fblocks);<br>
Opts.BlocksRuntimeOptional = Args.hasArg(OPT_fblocks_runtime_optional);<br>
Opts.Modules = Args.hasArg(OPT_fmodules);<br>
- Opts.ModulesDeclUse = Args.hasArg(OPT_fmodules_decluse);<br>
+ Opts.ModulesStrictDeclUse = Args.hasArg(OPT_fmodules_strict_decluse);<br>
+ Opts.ModulesDeclUse =<br>
+ Args.hasArg(OPT_fmodules_decluse) || Opts.ModulesStrictDeclUse;<br>
Opts.CharIsSigned = Opts.OpenCL || !Args.hasArg(OPT_fno_signed_char);<br>
Opts.WChar = Opts.CPlusPlus && !Args.hasArg(OPT_fno_wchar);<br>
Opts.ShortWChar = Args.hasFlag(OPT_fshort_wchar, OPT_fno_short_wchar, false);<br>
<br>
Modified: cfe/trunk/lib/Lex/ModuleMap.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/ModuleMap.cpp?rev=206027&r1=206026&r2=206027&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/ModuleMap.cpp?rev=206027&r1=206026&r2=206027&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Lex/ModuleMap.cpp (original)<br>
+++ cfe/trunk/lib/Lex/ModuleMap.cpp Fri Apr 11 06:47:45 2014<br>
@@ -243,8 +243,12 @@ void ModuleMap::diagnoseHeaderInclusion(<br>
resolveUses(RequestingModule, /*Complain=*/false);<br>
<br>
HeadersMap::iterator Known = findKnownHeader(File);<br>
- if (Known == Headers.end())<br>
+ if (Known == Headers.end()) {<br>
+ if (LangOpts.ModulesStrictDeclUse)<br>
+ Diags.Report(FilenameLoc, diag::error_undeclared_use_of_module)<br>
+ << RequestingModule->getFullModuleName() << Filename;<br>
return;<br>
+ }<br>
<br>
Module *Private = NULL;<br>
Module *NotUsed = NULL;<br>
<br>
Added: cfe/trunk/test/Modules/strict-decluse.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/strict-decluse.cpp?rev=206027&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/strict-decluse.cpp?rev=206027&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/Modules/strict-decluse.cpp (added)<br>
+++ cfe/trunk/test/Modules/strict-decluse.cpp Fri Apr 11 06:47:45 2014<br>
@@ -0,0 +1,9 @@<br>
+// RUN: rm -rf %t<br>
+// RUN: %clang_cc1 -fmodule-maps -fmodules-cache-path=%t -fmodules-strict-decluse -fmodule-name=XG -I %S/Inputs/declare-use %s -verify<br>
+<br>
+#include "g.h"<br>
+#include "e.h"<br>
+#include "f.h" // expected-error {{module XG does not depend on a module exporting 'f.h'}}<br>
+#include "i.h" // expected-error {{module XG does not depend on a module exporting 'i.h'}}<br>
+<br>
+const int g2 = g1 + e + f + aux_i;<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>
</blockquote></div><br></div></blockquote></div><br></div></div>
</blockquote></div></div></div><br></div></blockquote></div><br></div></div>
</blockquote></div></div></div><br></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>
</blockquote></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div>