<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Jun 16, 2017, at 11:38 AM, Ilya Biryukov <<a href="mailto:ibiryukov@google.com" class="">ibiryukov@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><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 style="word-wrap:break-word;line-break:after-white-space" class=""><div class=""><span class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class="">- Does the code for the refactoring engine use ASTUnit? Can it be used without that?</div></div></blockquote><div class="">Not really. The core engine itself doesn’t really care about the ASTUnit. As long as it gets the ASTContext from somewhere it should work :) <br class=""></div></span></div></div></blockquote><div class="">Good to know, then it shouldn't be a problem to reuse your code in clangd even if we move away from ASTUnit. </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;line-break:after-white-space" class=""><div class=""><span class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class="">- Does refactoring engine have a separate component for testing correctness of the refactoring? Does it have an API to provide conflicts?<br class=""></div></div></blockquote><div class="">Do you mean the correctness of the transformed source (i.e. it should be semantically identical to the original source) or something else? No, we don’t have a component that verifies semantic correctness. Furthermore, our implementation of “extract”s doesn’t provide any semantic correctness guarantees, as there are cases when the semantics of the program will change (we could probably diagnose them, but we don’t).<br class=""></div></span></div></div></blockquote><div class="">I.e. a common check for rename would be to resolve all occurrences again after rename is completed and check that they actually resolve to the renamed entity. (I.e. there might've been a parameter with the same name and some references may now resolves to a parameter, rather than some class you were renaming. In case there are errors, we may want to notify users about those (that's what I call conflicts).</div></div></div></div></div></blockquote><div><br class=""></div><div>Yes, this is a potential issue. We don’t have any code that performs this kind of verification though.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></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;line-break:after-white-space" class=""><div class=""><span class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="">- What data does refactoring engine requires from Indexing API specifically?</div></div></div></blockquote></span><div class="">What do you mean by the Indexing API? There’s no direct interaction with Clang’s indexing API and the refactoring engine. For global rename, Xcode’s indexer passes in the source location and symbol kinds for the previously indexed occurrences, but that relies on new refactoring APIs.</div></div></div></blockquote><div class="">I was asking about the data you pass from Xcode's indexer to refactoring engine. Got it, thanks.</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;line-break:after-white-space" class=""><div class=""><span class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="">- Why is 'Generate missing definitions' considered a cross-TU action? You actually touch only one file (i.e. the .cpp file where you put the definitions) and the AST for it should be enough for generating definitions.</div></div></div></blockquote></span><div class="">The action is typically initiated in the header file, so we treat it as a separate TU. Also, C++ classes can be scattered across many CPP files, so even if we are initiating in a CPP file that contains the declaration of the class, it might not be the right CPP file (we put the out-of-line functions to the CPP file that already has the majority of the out-of-line methods from that class).</div></div></div></blockquote><div class="">Got it.</div><div class="">Have you thought about actions that would require referring to classes/functions that aren't available in the TU you're changing (i.e. require includes to added in order to become available)?</div><div class="">Say, we want to implement an action/refactoring to create a missing declaration inside class A (see code below):</div><div class="">// Foo.h</div><div class="">struct A {</div><div class=""> int foo();</div><div class="">};</div><div class=""><br class=""></div><div class="">// Foo.cpp</div><div class="">#include "Foo.h"</div><div class="">#include <string></div><div class=""><br class=""></div><div class="">using namespace std;</div><div class="">int A::bar(string a) { // <-- action available here to create bar() in Foo.h</div><div class=""> return 10;</div><div class="">}</div><div class=""><br class=""></div><div class="">Can the current refactoring infrastructure be used to implement that? (i.e. add '#include <string>' to Foo.h, qualify string with 'std::' when creating a declaration?)</div><div class="">If not, maybe you've already thought how we could make that possible?</div><div class=""> </div></div>
</div></div>
</div></blockquote><br class=""></div><div>Good question. It should be possible to implement that using the new engine, you’d just have to teach it about <includes>. In your particular example we already have the “foo.h” included into “foo.cpp”, so we can do everything in one TU. I can think that an approach like this will work:</div><div><br class=""></div><div>- Gather declarations from imported/included headers an associate them with those headers (in that particular example `A` is from Foo.h and `std::string` is from string). </div><div>- Somehow determine their declaration status at the point of insertion (`A` is available, `std::string` isn’t). If a declaration isn’t available, we should include a header.</div><div><br class=""></div><div>I think that it should be possible to support this kind of workflow for cross-TU operation as well, we’ d just have to persist the pointers to declarations and their associated headers across TUs.</div><div><br class=""></div><div>I think the `string` will be qualified with `std` if you print it out, so I think the problem that we have here is actually the reverse - if you already include <string> and use namespace std in the header, then the `std::` prefix can be dropped. I believe there’s already existing code in lib/Tooling/Core that deals with that sort of thing, but I haven’t looked that much into it.</div><div><br class=""></div><div><br class=""></div><div><br class=""></div><div><br class=""></div><br class=""></body></html>