[clang] [clang-tools-extra] [clang][NFC] Un-constify `MultiLevelTemplateArgumentList` (PR #104687)

via cfe-commits cfe-commits at lists.llvm.org
Mon Aug 19 07:48:42 PDT 2024


Sirraide wrote:

> I am not sure removing const everywhere for one usage is a net-positive. There are no reason for template arguments to ever be modified.

I can’t say I know the full scope of this refactor (as in, what it would actually take to remove all of these `const_cast`s for good, because if I understand this correctly, this is only supposed to be the first step here because it ends up adding more `const_cast`s than it removes, from what I can tell), but I guess my two cents is that, while that statement makes sense to me, my understanding is that e.g. AST nodes in general aren’t really *supposed* to be modified after creation, but we have to do that in a bunch of places anyway. This just seems like another example of that.

I’m not sure keeping `const` everywhere and pretending like we don’t do that, only to then happily `const_cast` it away whenever it turns out that, actually, we *do* need to modify this thing after all, is really that helpful. Does `const` really mean anything at that point if, in practice, it ends up being used to signify that ‘this API *probably* doesn’t modify this thing, unless it has to, in which case it does’? 

I’m sure that’s not the case in every place we use `const`, but I’ve seen it often enough to where whenever I now see `const` somewhere on a function parameter in Clang, I’m never quite sure whether that function *actually* doesn’t modify that argument, or whether it’s just `const` because the argument just happens to be `const` everywhere we call that function.

> I would like to see explored a solution that clones MultiLevelTemplateArgumentList instead of mutating it in place

I might be missing a bit of the context here, but wouldn’t that have a performance impact? But, yeah, I suppose, as you said, someone would probably have to benchmark that.

https://github.com/llvm/llvm-project/pull/104687


More information about the cfe-commits mailing list