<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.MsoAcetate, li.MsoAcetate, div.MsoAcetate
{mso-style-priority:99;
mso-style-link:"Balloon Text Char";
margin:0in;
margin-bottom:.0001pt;
font-size:8.0pt;
font-family:"Tahoma","sans-serif";}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.BalloonTextChar
{mso-style-name:"Balloon Text Char";
mso-style-priority:99;
mso-style-link:"Balloon Text";
font-family:"Tahoma","sans-serif";}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri","sans-serif";}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Hi Richard,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I’ve put in the change for adding an ErrorRecovery flag to correctTypo. I’m currently working on getting the Modules tests to pass.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I’ve run into a snag with decldef.mm, which is pointing out a problem with my scheme that I thought I’d ask you about, and it also raised another question or
two.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">In the process of building a full global module index, there’s some kind of error in building the cxx_irgen_right and a few other modules, from the Inputs/module.map
file.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">My first question is: Are diagnostic message disabled while building modules? I don’t get any messages from the module build other than a “could not build
module” message. Or perhaps did I not set up something correctly in triggering the module build?<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Also, I noticed that when building the modules, the Inputs/module.map file was parsed with ModuleMap::parseModuleMapFile for each module build. Could ModuleMap
objects be shared?<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Finally, the problem I referred to earlier is that if a correctTypo call with ErrorRecovery == true occurs during a module build, the loadGlobalModuleIndex
function I added will be called in the module build thread, while the previous loadGlobalModuleIndex call that triggered the module build is still active. I’m supposing this in not a good thing, and could even trigger a deadlock via the file locking that
is done.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">What would be the best way to address this?<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I’m thinking perhaps have some “InGlobalModuleIndexBuild” flag somewhere to be checked in loadGlobalModuleIndex, and return either the existing index (perhaps
loading a fresh instance), or return null to trigger correctTypo to skip the missing import check. If this is the way to go, where would I put this flag, or otherwise how would I implement it?<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">The module build has its own CompileInstance object instance, so I can’t put it there. Or could it be a local static flag? Is there only one global module
index per run of clang? Or is there one per module.map file? I can’t put it in ModuleMap because of there being multiple instances of it. Could global module index objects be shared as well?<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Just to see if this recursion had some relationship to the module build error, I just put a temporary static flag in loadGlobalModuleIndex, but there was no
change with respect to the module build error message.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Sorry, I still don’t know as much as I should about the modules system, but I figured you could save me some time figuring this out.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Thanks.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">-John<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> metafoo@gmail.com [mailto:metafoo@gmail.com]
<b>On Behalf Of </b>Richard Smith<br>
<b>Sent:</b> Wednesday, March 26, 2014 5:13 PM<br>
<b>To:</b> Thompson, John<br>
<b>Cc:</b> reviews+D2671+public+d32f388120c4e27f@llvm-reviews.chandlerc.com; cfe-commits@cs.uiuc.edu<br>
<b>Subject:</b> Re: [PATCH] preview patch for fixit for finding modules needing import/inclusion<o:p></o:p></span></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<div>
<p class="MsoNormal">On Wed, Mar 26, 2014 at 4:19 PM, Thompson, John <<a href="mailto:John_Thompson@playstation.sony.com" target="_blank">John_Thompson@playstation.sony.com</a>> wrote:<o:p></o:p></p>
<p class="MsoNormal">Richard,<br>
<br>
Thanks for looking at this. I'll rework it.<o:p></o:p></p>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><br>
> We should probably only do this if we got to the end of typo-correction and found no corrections.<o:p></o:p></p>
</div>
<p class="MsoNormal">I'm not so sure about this. For example, in my test case, it would suggest "use_this2" as a typo fixit for "user_this1", instead of finding the symbol in the missing module import. I guess it's a question of whether an exact match should
take precedent over a partial match. What do you think? I'll follow your lead.<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I think a 'exact match, but missing import' correction is vastly more likely to be the one the user meant than a 'near match' correction, but I'm worried about the cost of doing this check. I suppose once we've committed to taking the performance
cost, it's better to do this lookup earlier.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Incidentally, I think you don't actually need to perform a lookup in your global module index code path -- just loading all the modules should be enough. Then the normal logic for finding hidden names from imported modules should find the
name.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt">> It looks like this will build every module described in any module map we've loaded. That could be extremely time-consuming, and could produce errors. Is that what you want here?<o:p></o:p></p>
</div>
<p class="MsoNormal">Yeah, that's a key question. The problem is that the global module index only has symbols for modules that clang with the current argument hash has built modules before, meaning that if someone forgets an import, they won't see this fixit
unless they've imported the module in some other compile with the same hash. So the key options are:<br>
<br>
1. Don't build a full global module index. User only sees the fixit for modules already in the global module index.<br>
2. When a symbol can't be resolved, create the full global module index on demand. (My current implementation.)<br>
3. Build a full global module index the first time or whenever the module.map file or headers change. Might be a slow build when this triggers.<br>
<br>
What do you think?<o:p></o:p></p>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">One use case we want to support has (almost) all header files in modules, including user headers that change frequently, so (3) is likely to be expensive for that use case, if I understand what you're proposing.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I'm torn between options 1 and 2. Mostly, I'm worried about the unpredictability of option 1.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">One other issue I just thought of: loading modules affects the semantics of the translation unit, even if those modules aren't made visible. Because a module is considered to be part of the program if it's loaded into a translation unit,
we'll diagnose mismatched redeclarations. For instance, if this is in a module that's in one of the module maps on your include path:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"> int foo;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">... and you hit a typo correction and then see this:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"> void foo();<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">... you're likely to get a 'redefinition as a different kind of entity' diagnostic.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">This is not ideal. The big problem is that we sometimes call CorrectTypo when we're not sure that an error has occurred! You can find these cases easily enough: look for cases where ErrorRecovery == false in the call to diagnoseTypo. I
suppose you could add an ErrorRecovery flag to CorrectTypo too, and only do your global module index import in the (common) error recovery case.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal">-----Original Message-----<br>
From: <a href="mailto:cfe-commits-bounces@cs.uiuc.edu">cfe-commits-bounces@cs.uiuc.edu</a> [mailto:<a href="mailto:cfe-commits-bounces@cs.uiuc.edu">cfe-commits-bounces@cs.uiuc.edu</a>] On Behalf Of Richard Smith<br>
Sent: Wednesday, March 26, 2014 3:25 PM<br>
To: <a href="mailto:dgregor@apple.com">dgregor@apple.com</a>; <a href="mailto:richard@metafoo.co.uk">
richard@metafoo.co.uk</a>; <a href="mailto:john.thompson.jtsoftware@gmail.com">john.thompson.jtsoftware@gmail.com</a><br>
Cc: <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
Subject: Re: [PATCH] preview patch for fixit for finding modules needing import/inclusion<br>
<br>
<br>
This looks like basically the right approach to me.<br>
<br>
<br>
================<br>
Comment at: lib/Sema/SemaLookup.cpp:4060-4061 @@ -4056,1 +4059,4 @@<br>
<br>
+ // Look for the symbol in non-imported modules.<br>
+ if (getLangOpts().Modules && getLangOpts().ModulesSearchAll) {<br>
+ // Try to find the type in modules via the global module index.<br>
----------------<br>
We should probably only do this if we got to the end of typo-correction and found no corrections.<br>
<br>
================<br>
Comment at: lib/Sema/SemaLookup.cpp:4096-4098 @@ +4095,5 @@<br>
+ TC.setCorrectionDecl(ModRes.getAsSingle<NamedDecl>());<br>
+ // Hide the module again. diagnoseTypo will unhide the decl module.<br>
+ Loader.makeModuleVisible(TheModule, Module::Hidden,<br>
+ TypoName.getLoc(), false);<br>
+ }<br>
----------------<br>
This won't work. We don't support visibility decreasing. Instead, you should be able to just load the module rather than making it visible, and tell your `LookupResult` to find hidden results during the lookup.<br>
<br>
================<br>
Comment at: lib/Frontend/CompilerInstance.cpp:1440<br>
@@ +1439,3 @@<br>
+ // Load a module as hidden. This also adds it to the global index.<br>
+ ModuleLoadResult Result = loadModule(TheModule->DefinitionLoc, Path,<br>
+ Module::Hidden, false);<br>
----------------<br>
It looks like this will build every module described in any module map we've loaded. That could be extremely time-consuming, and could produce errors. Is that what you want here?<br>
<br>
================<br>
Comment at: lib/Serialization/GlobalModuleIndex.cpp:345-358<br>
@@ -344,1 +344,16 @@<br>
<br>
+void GlobalModuleIndex::dump() {<br>
+ std::fprintf(stderr, "*** Global Module Index Dump:\n");<br>
+ std::fprintf(stderr, "Module files:\n");<br>
+ for (llvm::SmallVector<ModuleInfo, 16>::iterator I = Modules.begin(),<br>
+ E = Modules.end(); I != E; ++I) {<br>
+ ModuleInfo *MI = (ModuleInfo*)I;<br>
+ std::fprintf(stderr, "** %s\n", MI->FileName.c_str());<br>
+ if (MI->File)<br>
+ MI->File->dump();<br>
+ else<br>
+ std::fprintf(stderr, "\n");<br>
+ }<br>
+ std::fprintf(stderr, "\n");<br>
+}<br>
+<br>
----------------<br>
Feel free to just check this part in.<br>
<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D2671" target="_blank">http://llvm-reviews.chandlerc.com/D2671</a><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal">_______________________________________________<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>
<br>
<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><o:p></o:p></p>
</blockquote>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
</body>
</html>