<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 19, 2017, at 5:02 AM, Сергей Прейс <<a href="mailto:spreis@yandex-team.ru" class="">spreis@yandex-team.ru</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">Hello Alex,</div><div class=""> </div><div class="">This is great effort! I will monitor it closely for my employer in-house projects and will consider particitpation if find my help applicable. Do you plan any protection from incorrect source positions reported by clang in AST for some entities?</div></div></blockquote><div><br class=""></div><div>In the context of “Global Rename”, the engine shouldn't rename an identifier whose name is not the same as the name of the renamed symbol. This does not catch all cases though, as the incorrect source position might point to the same identifier. The number of incorrect source positions should be zero though, so we should just fix these issues.</div><div><br class=""></div><div>My stress-test tool that I mentioned in the RFC can actually help find such bugs, as it can detect cases when our AST misses a location for some symbol (by trying to initiate a rename at every non-keyword identifier token). Unfortunately it has a lot of false positives, so I haven’t had time to look into it that much.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""> I recently implemented c++ indexer and found at least one case where SourceLocation information doesn't match (<a href="https://bugs.llvm.org/show_bug.cgi?id=26195)" class="">https://bugs.llvm.org/show_bug.cgi?id=26195)</a>. I tried to find name reported in AST at SourceLoc attached to the entity. In some cases names are articificial and don't naturally match, but the case in question is real issue. There may be other cases (I don't index absolutely everyting in AST), so, probably, some protection logic is needed agains similar issue. I have fix for this particular issue with test exhibiting the offending behavior using c-index-test on trunk clang (<a href="https://reviews.llvm.org/D32439)" class="">https://reviews.llvm.org/D32439)</a>, so you may look and see how it is relevant to your development.</div><div class=""> </div></div></blockquote><div><br class=""></div><div>Thanks! I’ll take a look this week.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="">Thak you,</div><div class="">-- </div><div class="">Serge Preis</div><div class=""> </div><div class=""> </div><div class=""> </div><div class=""> </div><div class="">17.06.2017, 02:00, "Alex Lorenz via cfe-dev" <<a href="mailto:cfe-dev@lists.llvm.org" class="">cfe-dev@lists.llvm.org</a>>:</div><blockquote type="cite" class=""><div class=""> <div 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> <div class=""><div class=""><div class=""><div class=""><blockquote style="margin:0 0 0 0.8ex;border-left:1px #ccc solid;padding-left:1ex;" class=""><div class=""><div class=""><blockquote type="cite" class=""><div class=""><div class=""><span class="">- Does the code for the refactoring engine use ASTUnit? Can it be used without that?</span></div></div></blockquote><div class=""><span 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 :) </span></div></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 style="margin:0 0 0 0.8ex;border-left:1px #ccc solid;padding-left:1ex;" class=""><div class=""><div class=""><blockquote type="cite" class=""><div class=""><div class=""><span class="">- Does refactoring engine have a separate component for testing correctness of the refactoring? Does it have an API to provide conflicts?</span></div></div></blockquote><div class=""><span 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).</span></div></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 class=""> </div><div class="">Yes, this is a potential issue. We don’t have any code that performs this kind of verification though.</div> <blockquote type="cite" class=""><div class=""><div class=""><div class=""><div class=""><div class=""> </div><blockquote style="margin:0 0 0 0.8ex;border-left:1px #ccc solid;padding-left:1ex;" class=""><div class=""><div class=""><blockquote type="cite" class=""><div class=""><div class=""><div class=""><span class="">- What data does refactoring engine requires from Indexing API specifically?</span></div></div></div></blockquote><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 style="margin:0 0 0 0.8ex;border-left:1px #ccc solid;padding-left:1ex;" class=""><div class=""><div class=""><blockquote type="cite" class=""><div class=""><div class=""><div class=""><span 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.</span></div></div></div></blockquote><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=""> </div><div class="">// Foo.cpp</div><div class="">#include "Foo.h"</div><div class="">#include <string></div><div 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=""> </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></div><div class="">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 class=""> </div><div class="">- 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 class="">- 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 class=""> </div><div class="">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 class=""> </div><div class="">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 class=""> </div><div class=""> </div><div class=""> </div><div class=""> </div></div>,<p class="">_______________________________________________<br class="">cfe-dev mailing list<br class=""><a href="mailto:cfe-dev@lists.llvm.org" class="">cfe-dev@lists.llvm.org</a><br class=""><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a></p></blockquote></div></blockquote></div><br class=""></body></html>