[cfe-dev] My first patch to clang

[John McCall]

On Dec 4, 2009, at 8:37 AM, Nicola Gigante wrote:

> Hello,
> 
> as promised I've started to look at how clang works under the hood.
> 
> My first "kid job" was to implement a simple diagnostic improvement that suggests to use -> instead of . if the base expression is a pointer. You find the patch attached.

Great idea!  We have some similar-in-concept diagnostics/fixits, but not this one specifically.

I see a few issues with this patch, mostly minor.

The first is that we like to have reasonably high confidence in our suggestions.  There are plenty of pointer base types where changing '.' into '->' won't actually help;  for example, int*, struct foo**, etc.  We shouldn't recommend using '->' unless the base is specifically a pointer to a record type.

The second is that fixits should be aligned with error-recovery whenever possible.  In this case, that means that if we're doing to give a fixit for transforming the '.' into the '->', we should then proceed to do the member lookup, build the expression, etc. as if the user had typed '->'.  It looks like you can make that work very easily:  just (1) pass IsArrow to LookupMemberExpr by reference instead of by value, (2) try to diagnose this error *before* the block in LookupMemberExpr that calls LookupMemberInRecord, and (3) set IsArrow = true when you diagnose.

> In the last mail, Douglas told me to send trivial patches to cfe-commits but I've prefered to post it here because it's the first one and I'm unsure about a couple of things.
> 
> 1) Have I chosen the right locations for the caret, the range and the substitution? The original error used to put the caret _after_ the dot, but I put the caret _at_ the dot, because I think it's more reasonable.

Your locations look good.  Pointing the caret at the operator is fine in this case, since the presumed error is with the operator, not with the member or the base expression.

You should test with -fixit to make sure your suggestion actually works.

> 2) Is it ok to add a new diagnostic string in this case? Are the string and the enum name appropriate?

They look fine.

Thanks!

John.