martinboehme wrote:

Drive-by comment.

> I think the issue is that the downstream code doesn't expect to `nullptr_t` 
> to have a pointer value / modeled nullability state until there is a cast 
> (e.g., comments around 
> https://github.com/google/crubit/blob/859520eca82d60a169fb85cdbf648c57d0a14a99/nullability/pointer_nullability_analysis.cc#L1130)

I think this is the correct way to model things, and it shouldn't be changed. 
`nullptr_t` is not a pointer type, so it should not be associated with a 
`PointerValue`.

Note that if you compare a pointer to `nullptr`, the `nullptr` is cast to the 
pointer type ([godbolt](https://godbolt.org/z/335GoM94v)), so the only case 
that needs to be treated specially is `nullptr == nullptr`. This might come up 
in macro or template code, so it does seem worth getting right.

> to handle 'nullptr == nullptr' could special case that in 
> `evaluateBooleanEquality` (e.g., check if LHS and RHS 
> getType().isNullPtrType()) -- instead of creating PointerValues in 
> VisitCXXNullPtrLiteralExpr

This is one way of doing this, and I don't really see any issues with this.

If we gave `nullptr` an actual `Value` though, that would require less 
special-casing. (The code you have in this PR would simply do the right thing.)

I don't think it's worth creating a new `Value` subclass for `nullptr`. Just 
add a method `Value &getNullptrValue() const` to `Environment` that always 
returns the same `Value` object (managed by the `Arena`), and use 
`getNullptrValue()` when evaluating a `CXXNullPtrLiteralExpr`. I think this 
would be a nice local fix.

https://github.com/llvm/llvm-project/pull/129502
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to