<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Mar 26, 2014 at 4:19 PM, Thompson, John <span dir="ltr"><<a href="mailto:John_Thompson@playstation.sony.com" target="_blank">John_Thompson@playstation.sony.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Richard,<br>
<br>
Thanks for looking at this. I'll rework it.<br>
<div class=""><br>
> We should probably only do this if we got to the end of typo-correction and found no corrections.<br>
<br>
</div>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.</blockquote>
<div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
> 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>
</div>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?<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>I'm torn between options 1 and 2. Mostly, I'm worried about the unpredictability of option 1.</div><div><br></div><div><br></div><div>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:</div>
<div><br></div><div> int foo;</div><div><br></div><div>... and you hit a typo correction and then see this:</div><div><br></div><div> void foo();</div><div><br></div><div>... you're likely to get a 'redefinition as a different kind of entity' diagnostic.</div>
<div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
-----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><br>
</div></div>_______________________________________________<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><br>
</blockquote></div><br></div></div>