aaron.ballman added a comment.

First off, thank you so much for your patience while I took the time to think 
about this. I know it can be frustrating to not hear review feedback in a 
timely manner, so I'm sorry for that.

I've been sitting on this for a while because type attributes are a complex 
beast. There's the attribute parsing component to it, which is generally pretty 
straightforward and is largely what's handled here in your patch. But the 
bigger issue is the effects on the type system, which is one of the only ways 
to make a type attribute particularly useful. Unfortunately, type system 
effects are scattered all over the code base and can have drastic impact on 
compiler performance (template instantiation depth limits, memory overhead 
pressures, etc), and we tend to have quite a few assertions in the compiler to 
help users catch the places they need to update.

Given that utility from this would require significant changes in Clang itself, 
I'm not certain what a plugin buys us in terms of functionality compared to the 
risk of type attribute plugins making the compiler somewhat unstable in terms 
of performance and potentially even assertions, etc. tl;dr: type attributes 
aren't easily pluggable yet so by the time you make them useful, you basically 
have a clang fork instead of a plugin augmenting Clang. Based on that, I'm 
inclined to not ask you to do further work on it (that work would be akin to me 
asking you "please rework large parts of the type system" which would be a high 
risk activity and not a reasonable request as part of this patch). However, I 
also wanted to hear what your ideas are on how you were expecting to use the 
functionality here from the patch, as maybe there's utility that I'm not 
thinking of.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114235/new/

https://reviews.llvm.org/D114235

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to