sammccall added a comment. Sorry for taking a while to get to this...
I'm not sure this is really a helpful use of Context. (Though some would argue that there are no good uses, my manager was appalled when we added it...) Does this enable some future work, or is it reasonable to evaluate this patch in isolation? There's of course no hard-and-fast rules but my checklist is: - is this some kind of optional input, like overriding some behavior? (i.e. forgetting it isn't terrible, and we needn't burden all callers) - does the parameter otherwise need to be plumbed through many layers, most of which don't use it? - will parameter otherwise need to be added to public interfaces where they don't make sense? Here it's a mandatory input, and the alternative is cluttering a couple of layers of purely-private interfaces, so this seems like a lot of magic to avoid a fairly small amount of awkwardness. (The original use cases for context were around VFS state for embedders, where *all* these things were true - the lspEncoding() is another place where this is IMO a pretty good tradeoff) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92290/new/ https://reviews.llvm.org/D92290 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
