Coding style of SVals - dyn_cast<T>(SVal) or SVal.getAs<T>()

Which casting style should we pursue?

We already use the dyn_cast style with other analyzer-specific types, such as MemRegions and SymExprs. And I’m about to land the by-value dyn_casts for SVals in D125709.
At this point, the getAs and castAs member functions would became redundant.
We should consider dropping them in favor of dyn_cast<T> and cast<T> to make it more consistent with how we cast MemRegions and SymExprs.

Alternatively, we could introduce MemRegion::getAs<T>() and SymExpr::getAs<T>() just making a syntactic sugar for dyn_cast and prefer using those.

IMO this is worth a discussion since consistency is important.

Unfortunately, we currently don’t have an isa<Ts...>() member function variant, but if we choose the member function style D125749 implements it.


In addition to this, I would prefer not to spell out the type in the following scenarios.
So I would prefer this:

auto Var = dyn_cast<T>(V);
auto Var = cast<T>(V);

Compared to this:

Optional<T> Var = dyn_cast<T>(V); // or V.getAs<T>()
T Var = cast<T>(V); // or V.castAs<T>()

What’s your opinion?
@NoQ @Xazax-hun @martong @ASDenysPetrov @Szelethus

Some prior discussion in the area: ⚙ D33672 [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

Basically the SVal class is pretty unique in how it combines non-trivial inheritance hierarchy with pass-by-value. This is only possible because all subclasses are of the same size (in rare cases when we run over the limit, out-of-line storage is used). This is a very unusual situation and I don’t think consistency can be expected.

Both castAs<>()/getAs<>() and isa<>()/cast<>()/dyn_cast<>() are used by other classes but they have completely different meaning there. In particular, SVals are supposed to be copied by these functions, whereas other classes aren’t.

I don’t see much benefit in transitioning from one to the other; they’ll still have nothing to do with actual functions with the same name in other classes. I’d rather have them have completely different names to attract attention to different semantics.

If we really want this, I’d advocate for having getAs()/castAs() as member functions and isa()/cast()/dyn_cast() as global functions, because this matches other classes’ behavior.

I think it would be great to have a uniform way of casting in the CSA codebase. And I vote for the traditional global isa<T>, cast<T> and dyn_cast<T> functions.

My understanding is that D125709 implements these functions in a way that they do copy. I agree that the cast-by-value thing is unique in the llvm-project code-base, however, it is still a cast operation. And as such, perhaps it should follow the traditional LLVM casting conventions.