However, the upstream tool repo hasn’t been updated in two years, and it is reported to not even be compiling anymore since Clang 19.
The last time it’s Clang tests have been directly updated was in D115521, and since then, there has only been updates due to incidental/indirect non-intentional changes in how our template instantiation contexts are operated.
The main reason for me to propose such a removal is because Templight tests are very painful to update.
That’s completely full of matches on source locations.
The CHECK lines are interleaved with the source code.
If clang gains or loses template instantiation contexts, then you have to add more CHECK lines to cover those, but that changes the source code lines of every subsequent test.
The feature included no tool for automatically updating these tests.
I think for an abandoned tool, it adds quite a bit of maintenance burden, so I’d like to propose for us to let it go, unless it starts being maintained again.
I would much prefer a call for a new maintainer first and then removal if no one steps up. I do understand the burden and if no one steps up then perhaps the burden is not worth it.
If someone is interested in maintaining it, I would not be opposed to keeping it in the tree. However, if it continues to have no active maintainers, I think it is reasonable to consider removal now that it’s becoming more of a maintenance burden. (I would be especially curious to know if this reduces template instantiation overhead in Clang itself due to no longer calling out to observers; that would strengthen the case for removal.)
That said, do we know if any other projects are using these facilities beyond Templight? I’m not seeing evidence with my searches: context:global lang:C++ … - Sourcegraph but good to see if any downstreams are using this for their own needs before removing it.
I honestly had no idea this existed. I would think that maintainer or not, unless we can show we actually have users of this, we should abandon it.
I like the idea of it, but unless we have users that say they are using it (and the disclaimer on the templight github is still “a very preliminary state” makes me think not.
Personally, I no longer use Templight (mostly because C++17 and later makes obsolete most of the heavy template meta-programming techniques that you’d need templight for). I believe it gets a fair amount of usage, sporadically, judging by stars and issues and all that, but it’s obviously niche and not something that people would use in CI/CD contexts, mostly one-off debugging / profiling. Maintaining it on both sides of the fence is indeed difficult. For example, the templight tool has to basically maintain a modified version of the clang front-end (aka “clang_driver.cpp” → “templight_driver.cpp”) which has been very brittle because that particular code changes often between clang versions and it’s a huge mess, I have long since stopped trying to keep that up-to-date. And I’m out of the loop as far as maintenance inside LLVM, but I can imagine and see that it is burdensome.
Long story short, I don’t object to removing support. The project is effectively stuck on older versions already. And I don’t think it makes much sense anymore in a post-constexpr / post-concept world.
That’s part of the templight support. The problem is maintaining them after templight is gone, as I believe at that point there would be no in-tree users left.
It’s also worth pointing out that it seems like most of the internal maintenance burden is about the source location matches. It might make sense to simply remove the exact line numbers from the checks.
The risk that line-numbers are wildly off is very low (and I assume, tested in other ways already, e.g. diagnostics) because the templight hooks are not doing anything special with that, just forwarding the same point-of-instantiation/definition source locations that a typical template instantiation error diagnostic message would. It doesn’t really serve much purpose to verify that the templight entries produced have the correct source locations.
Looking at the original patch discussion, there was some degree of interest in the fact that the templight-related tests act as a sanity check on what gets instantiated in different scenarios.
I am relying on TemplateInstCallbacks. Would that be removed as a part of this?
Yes, that is mainly what is proposed. That’s the core “hook” that was introduced to make templight (and metashell) possible. It seems the tests of that hook is the troublesome bit from a maintenance standpoint. And, of course, if you remove the tests you have to remove the hook too, you can’t leave it in and not have a test for it. Maybe we can just make the test less strict. I’m also curious what you use this hook for? I wasn’t aware anyone else was using those internal hooks.
I am using TemplateInstCallbacks to record the template parameters of instantiations of a “user supplied” visitor function template for pattern matching. This would be possible with mutable reflection variables which I think are not going to be in C++ any time soon. I would be willing to help out if needed to make the tests less brittle if it meant keeping TemplateInstCallbacks around.
I also have an extension to PragmaHandler that provides access to clang::Parser that I was going to RFC once I wrote some tests. Perhaps it could include a test of the use of TemplateInstCallbacks (as an example) if that was necessary.
You don’t need to go as far as removing the source locations from the tests.
Simply making sure there is no source code after a templight dump suffices.
This could be easily accomplished by moving all the dumps together to the end of all test files.
If that gets hard to read, then split all test cases into different files.
I think that’s a use beyond what we can reasonably claim to support long term.
Ultimately, the TemplateInstantiation contexts are a mechanism clang uses to produce notes for diagnostics in a RAII style manner. I think that’s the only thing that can be relied on, as long as we keep the current approach of producing (potentially tons) of context notes for each instantiation recursively, and don’t come up with an approach that’s more sensible from consumer readability and compiler performance points of view.
It has grown way beyond just notes about template instantiations too, and I always wonder for most of them if we should have exposed them on these callbacks at all.
It has also invited all kinds of unreliable hacks within clang to be built on top of it. Which all take a long time to solve, so a lot of it is not going away any time soon.
We will skip adding these contexts, for cases where we don’t need to produce a note, and where hacks don’t need them, and for cases where we lazily instantiate.
I’m not sure I fully understand the use-case, but it sounds like something that could be accomplished on the AST alone, i.e., with an AST matcher (ala clang-tidy checks) and only using libclang rather than invoking a full compilation pass. Maybe it’s just easier through the templight hooks (believe me, I know how hard it can be to write AST pattern matchers), but it feels like an abuse of those hooks.
Ok, thanks! The Clang Area Team discussed this today and though we didn’t officially accept this, I believe your retraction of objection is sufficient to have us accept this. Thank you for your quick response!
This RFC was accepted in this message.