<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 6 January 2015 at 15:03, Mikael Persson <span dir="ltr"><<a href="mailto:mikael.s.persson@gmail.com" target="_blank">mikael.s.persson@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div>Hi Nick,</div><div><br></div><div>Glad to finally get a sign of life from the clang dev team!</div></div></div></blockquote><div><br></div><div>I'm more on the LLVM side. While I'm willing to make some changes to clang I'll sometimes conclude that I don't know an area of clang well enough to approve a patch, on a case by case basis.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><span class=""><div>> <span style="font-size:13px">The llvm project requires that patches be submitted by their author (or authors). If you grabbed the original templight code and refactored it, then it may be coauthored between you and the original authors. Our policy requires that an author submit their own code, not mail in someone else's</span><br></div><div><span style="font-size:13px"><br></span></div></span><div><span style="font-size:13px">The patches from this email are very out-dated (and I was completely unfamiliar with the process of patch submissions... well, I still am to be honest ;) ). I submitted a much smaller patch after successfully managing to extract all the templight code out of the clang source, to make it a separate tool instead, leaving only very minor "hooks" to be added to Clang itself. That patch can be found at: </span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">http</span>://<a href="http://reviews.llvm.org/D5767" target="_blank">reviews.llvm.org/D5767</a></div><div><br></div><div>As for the authorship, there are a few things to notice. </div><div><br></div><div>First of all, in the new patch that I submitted, all of the code is my own code, because all of the original templight code has been essentially factored out from the original patch, i.e., the "real" templight code now sits in a separate project at: </div><div><br></div><div><a href="https://github.com/mikael-s-persson/templight" target="_blank">https://github.com/mikael-s-persson/templight</a></div><div><br></div><div>There is also a more up-to-date clang patch on there, which I will re-submit eventually to Clang when there is more sign of life from the community (I don't like wasting my valuable time throwing patch-updates into a bottomless pit).</div><div><br></div><div>The original patch also lacked any specific authorship notice and licensing. But I'm more than happy to add any co-author that is needed to satisfy your submission requirements. All I want is the patch to go through, and I think all other interested parties and original authors feel the same. And after all, we are only talking about of few dozen trivial lines of code here.</div><div><br></div><div>And for the record, I'm a bit annoyed by the "<span style="font-size:13px">mail in someone else's code", as it kind of belittles the massive refactoring work I did on the original patch, and the fact that the majority of what it is now is my own code.</span></div></div></div></blockquote><div><br></div><div>My sincere apologies, I did not intend to belittle. I was only summarizing what the policy says, not making any claim about the work you have done. If you tell me that the code meets the requirements in the developer's policy, that is enough for me.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><span class=""><div>> <span style="font-size:13px">I think that ActiveTemplateInstantiation should be changed to a form of InstantiationRecord and held from the relevant AST nodes.</span><br></div><div><span style="font-size:13px"><br></span></div></span><div><span style="font-size:13px">Maybe so, I don't know, and frankly I don't care. Well, I do care for having good template instantiation code in Clang, but I cannot afford to care too much about design decisions that are not mine to take.</span></div></div></div></blockquote><div><br></div><div>Ok. It matters to me due to code refactoring tools.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><span class=""><div><span style="font-size:13px">> </span><span style="font-size:13px">The changes to ATI to pull it out of Sema and make it part of the AST are complicated enough, I think you should send that out as a first patch.</span><span style="font-size:13px"> </span><br></div><div><span style="font-size:13px"><br></span></div></span><div><span style="font-size:13px">I think there is a confusion here. I never pulled the ATI into the AST, I only pulled it out of the Sema class. It has literally just gone from being a nested class within Sema to being a class at global scope (clang namespace, of course!), that's all. There is no change whatsoever in the code except for a change in scope and header-file placement (to avoid including the Sema header, and just because nested classes are a bad idea (I come from Boost library development, the design guidelines are more strict over there)).</span></div></div></div></blockquote><div><br></div><div>My mistake, I was thinking that you pulled it out of Sema-the-library, not Sema-the-class. In particular, it still uses sema::TemplateDeductionInfo.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div><span style="font-size:13px">If you want to work on making ATIs a part of the AST, you're welcome to try / propose it. It is none of my concern, and in fact I would object to it (see below). </span></div><span class=""><div><br></div><div>> <span style="font-size:13px">There's also template instantiation for different reasons which are not currently recorded (ie., due to overload resolution, due to virtual methods). Since that's a change to ATI, that would logically go next.</span></div><div><span style="font-size:13px"><br></span></div></span><div><span style="font-size:13px">You're right, the categories of template instantiations are too broad. That is something I would be interested in discussing and intend to bring up after my patch has gone through. The cases of overload resolution and virtual methods are not exactly the first ones that come to mind for me though. The additional categories I was going to propose are for differentiating class template instantiations, function declaration instantiations and function definition instantiations. The lack of categorization of those things currently cause templight traces to have ambiguous entries, which I would like to avoid.</span></div><span class=""><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">> </span><span style="font-size:13px">I think we usually use "Callbacks" instead of "Observer", similar to PPCallbacks?</span></div><div><span style="font-size:13px"><br></span></div></span><div>Fine. When we get closer to getting this (D5767) patch through, that can easily be changed, no problem.</div><div><br></div><div>> <span style="font-size:13px">At a high level, I think that what we currently do in Sema by tracking template instantiations as a stack </span><span style="font-size:13px">should be available in the AST after instantiation is complete.</span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">I'm not sure if this makes sense in terms of the AST, after all, the stack of template instantiations are compilation steps, not actual entities in the code. My understanding is that the AST is only there to record all the entities in the code. I see this as a case of semantic pollution. What other records of compilation steps are recorded in the AST?</span></div></div></div></blockquote><div><br></div><div>Implicit casts, for one. Expressions in the C++ standard have associated grammar therefore implicit casts are not expressions, even though we call them ImplicitCastExpr in clang. We could remove them and have the consumers of the AST imagine them as needed, saving memory and possibly compilation time. Or possibly costing compile time, if removing them would make Clang's own code more complicated or force us to check more conditions and recompute properties more often.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div><span style="font-size:13px">Also, if the AST is to keep a record of all the template instantiations that occurred, this could significantly increase memory consumption (and thus, speed as well).</span></div></div></div></blockquote><div><br></div><div>Yes. I am also concerned about this. We had the same concern about keeping around source locations and macro expansions, but it turns out that clang was able to make it work efficiently. Similar efficiency would be a requirement, and no, I don't know how to do it. I'm assuming we would figure it out.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div><span style="font-size:13px"> It is a good thing that the compiler throws away this information because it would and could amount to a lot if it kept it all (I would say approximately 5-10% increase in memory consumption, from my experience). This would be a lot of passive overhead (and therefore, would have to be disabled by default, of course). I certainly would not want such overhead by default in any compiler that I use (and I'm not sure that people who use libclang would want this either).</span></div></div></div></blockquote><div><br></div><div>Part of the question is whether we could use it to simplify code in Sema itself. I don't think anyone wants it to be an optional feature, it should either be efficient enough to be always-on, or not there at all.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div><span style="font-size:13px">> </span><span style="font-size:13px">Then we could query the AST to ask why something was instantiated instead of tracing the act of instantiation. Is there any use case for templight where this wouldn't work?</span><br></div><span class=""><div><span style="font-size:13px"><br></span></div></span><div><span style="font-size:13px">Yes, pretty much all of templight as a profiler would not work that way, unless the AST also held records of time- and memory- consumption during those instantiation steps. The AST is useful to the templight debugger and is also used in conjunction with the templight traces by the metashell utilities (that Zoltan linked to). The AST describes the code, the templight traces describe the compilation process, they are entirely orthogonal features (yet complementary), as far as I'm concerned.</span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">To me, it is important to stress that, although the debugging and interactive querying of the AST is a neat feature, the real "production-critical" feature here is the profiling. This is my primary interest, and those of my peers, i.e., the people who write production template-heavy code that need to optimize the compilation time and memory. Focusing too much on the debugging and AST querying aspects could impede that primary goal, as far as I'm concerned. </span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">And as Zoltan mentioned, there could be issues with the safe-mode and dealing with failures to instantiated the templates if we are going to rely on "successful" AST nodes to describe the compilation process. </span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">There are basically three use-case categories for templight (from most "novice" to most "professional"): learning, debugging and profiling. Learning and debugging imply that things might be failing a lot, and profiling imply that there is no need to understand the process but a need to identify performance bottlenecks. Your approach does not serve any of these domains particularly well.</span></div></div></div></blockquote><div><br></div><div>This is really enlightening, thanks.</div><div><br></div><div>I'm convinced that my idea of moving template instantiation information into the AST is orthogonal to the needs of templight. Also, I think the observer you want to add is a fine idea and we should add that.<br></div><div><br></div><div>Nick</div></div></div></div>