[PATCH] D147073: [Coverage] Handle invalid end location of an expression/statement.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D147073#4242566 <https://reviews.llvm.org/D147073#4242566>, @hans wrote:

> In D147073#4240793 <https://reviews.llvm.org/D147073#4240793>, @zequanwu wrote:
>
>> In D147073#4240017 <https://reviews.llvm.org/D147073#4240017>, @hans wrote:
>>
>>> I'm not familiar with this code. I suppose the question is whether it's reasonable for this code to expect that the source locations are always valid or not?
>>
>> Yes.
>>
>> For `0 ? T<C>{} : T<C>{};`, the both branches have valid start location but invalid end location. See comments at https://github.com/llvm/llvm-project/issues/45481#issuecomment-1487267814.
>>
>> For the `std::strong_ordering`, I found that `DeclRefExpr` in the ConditionalOperator's true branch has invalid start and end locations. It might because it's inside a `PseudoObjectExpr`. Maybe we should simply just skip visiting `PseudoObjectExpr`, I see that its begin and end location are the same. I'm not familiar with that expression, so, I just handled it by adding checks for validating begin and end locations.
>
> Right, I'm just wondering if it's reasonable that this code should have to handle such situations. (It seems it can't handle it perfectly anyway, since the coverage won't be completely correct.) Maybe Aaron can comment on what the expectations are for these locations.

`PseudoObjectExpr` is a strange little beast in that it's an abstract object in the AST. It gets used when there's is a particular syntax that is modelled directly by the language as a series of expressions (e.g., writing `foo->x = 12;` in the source, but due to some language feature like `__declspec(property)`, the semantics are `foo->setX(12);` as a member function call instead). So it has multiple source locations that are of interest and you have to know what information you're after -- the syntactic location or one of the semantic locations. I'm not super familiar with the code coverage machinery, but it looks like it's walking over the semantic expressions rather than the syntactic one, and that seems a bit fishy to me because I'd assume that coverage is intended to describe what the user actually wrote in their source. So I don't think I'd skip the `PseudoObjectExpr`, but you might need to handle it differently in `CounterCoverageMappingBuilder` by adding a `VisitPseudoObjectExpr()` method and not walking over *all* of the children, but only the syntactic form of the expression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147073