Restart handling of a function declarator if the function name was typo-corrected

Not sure if this is the right way (without a major rewrite of Sema::CorrectTypo and friends) to achieve the goal, but it solves the issue brought up by Jordy in response to r137966 where an out-of-line function definition is not rechecked against existing declarations to see if it is valid after being typo-corrected.

[jediknil_belkadan.com, 2011-08-21 05:34:17]

I'm not a fan of emitting errors for code the user didn't write, but I'd be okay
with it if there was a fixit for the presence or absence of 'const' as well.
Then we could make a two-step recovery.

Actually, a fixit for const mismatches is probably a good idea anyway...

Jordy


On Aug 19, 2011, at 13:47, rikka@google.com wrote:

> Reviewers: chandlerc,
> 
> Description:
> Not sure if this is the right way (without a major rewrite of
> Sema::CorrectTypo and friends) to achieve the goal, but it solves the
> issue brought up by Jordy in response to r137966 where an out-of-line
> function definition is not rechecked against existing declarations to
> see if it is valid after being typo-corrected.
> 
> Please review this at http://codereview.appspot.com/4929043/
> 
> Affected files:
>  M     lib/Sema/SemaDecl.cpp
>  M     test/SemaCXX/function-redecl.cpp
> 
> 
> <issue_4929043_patch.diff>_______________________________________________
> cfe-commits mailing list
> cfe-commits@cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

[Kaelyn, 2011-08-22 16:30:45]

I agree that a fixit for const mismatches would be nice too, but the focus
of this patch is to address the situation where a function name is mistyped
and there is some other mismatch (such as const) that would cause the
typo-corrected function name to still not match an existing function
declaration. Right now clang will correct the typo but not let the user know
that there is still a problem (this is also not the only case where typo
correction can expose/generate additional error messages).

Cheers,
Kaelyn

On Sat, Aug 20, 2011 at 10:34 PM, Jordy Rose <jediknil@belkadan.com> wrote:

> I'm not a fan of emitting errors for code the user didn't write, but I'd be
> okay with it if there was a fixit for the presence or absence of 'const' as
> well. Then we could make a two-step recovery.
>
> Actually, a fixit for const mismatches is probably a good idea anyway...
>
> Jordy
>
>
> On Aug 19, 2011, at 13:47, rikka@google.com wrote:
>
> > Reviewers: chandlerc,
> >
> > Description:
> > Not sure if this is the right way (without a major rewrite of
> > Sema::CorrectTypo and friends) to achieve the goal, but it solves the
> > issue brought up by Jordy in response to r137966 where an out-of-line
> > function definition is not rechecked against existing declarations to
> > see if it is valid after being typo-corrected.
> >
> > Please review this at http://codereview.appspot.com/4929043/
> >
> > Affected files:
> >  M     lib/Sema/SemaDecl.cpp
> >  M     test/SemaCXX/function-redecl.cpp
> >
> >
> > <issue_4929043_patch.diff>_______________________________________________
> > cfe-commits mailing list
> > cfe-commits@cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>

[Kaelyn, 2011-08-24 20:07:16]

Rietveld's upload.py seems to not have included the patch or description
when I used it to upload a new patchset to an existing issue. :'(

The new patch makes use of SFINAETrap to only accept a typo correction and
restart the function declaration handling if doing so doesn't trigger
additional errors--so no correcting to a declaration with mismatched const
qualifiers, etc. While adding the SFINAETrap usage, I also renamed
isNearlyMatchingFunction to hasSimilarParameters as I kept having to go look
at its body to remember if it checks anything about the two function
declarations other than the parameter list. I figured hasSimilarParameters
was a much better name for a function that checked if two FunctionDecls take
the same number of parameters and that the types of the parameters in each
position are the same or nearly so.

Cheers,
Kaelyn

On Wed, Aug 24, 2011 at 12:57 PM, <rikka@google.com> wrote:

>
http://codereview.appspot.com/**4929043/<http://codereview.appspot.com/4929043/>
>

[chandlerc, 2011-08-24 23:13:06]

Thanks for working on this. I wonder if we need to generalize this whole pattern
somewhere, as its a lot of work. But it sure seems to pay off. =D

http://codereview.appspot.com/4929043/diff/5001/lib/Sema/SemaDecl.cpp
File lib/Sema/SemaDecl.cpp (right):

http://codereview.appspot.com/4929043/diff/5001/lib/Sema/SemaDecl.cpp#newcode...
lib/Sema/SemaDecl.cpp:2909: /// hasSimilarParameters - Determine whether the C++
functions Declaration
s/functions/function's/

http://codereview.appspot.com/4929043/diff/5001/lib/Sema/SemaDecl.cpp#newcode...
lib/Sema/SemaDecl.cpp:4205: // The rest of the parameters are for calling
ActOnFunctionDeclarator
Ugh. Could we stash these in a parameter object, or somehow factor it so that
they're more clearly managed? That would also help with the confusing naming of
'S' and 'Sc' for example.

Also, this function definitely needs a doxyment.

http://codereview.appspot.com/4929043/diff/5001/lib/Sema/SemaDecl.cpp#newcode...
lib/Sema/SemaDecl.cpp:4244: // Back up the value of the by-reference booleans
I have a slight preference to call the function with our own local variables,
and then to set the output parameters if it succeeds... Makes for nice names
like "TrialRedeclaration" and "TrialAddToScope"

http://codereview.appspot.com/4929043/diff/5001/lib/Sema/SemaDecl.cpp#newcode...
lib/Sema/SemaDecl.cpp:4284: if (TypoWasCorrected)
Why not just hoist this into the code that sets TypoWasCorrected to true?

Hmm, at this point in this function's complexity, I think we need to split it
into several functions. Specifically I'm thinking the loop below to emit notes
should be a separate function that we can call from the various places where it
makes sense, and then we can hoist these diagnostics up into the branches above,
and switch them to use early exit, reducing indentation.

Does that make sense? Separate patches would be fine with me here, and those
should be fine for post-commit review.

http://codereview.appspot.com/4929043/diff/5001/lib/Sema/SemaDecl.cpp#newcode...
lib/Sema/SemaDecl.cpp:4290: S.Diag(NewFD->getLocation(), DiagMsg) << Name <<
NewDC << NewFD->getLocation();
80 columns

[Kaelyn, 2011-08-25 00:14:54]

Chandler, I'd like to get any additional comments you have about the function
parameters before sending out a new patch as I suspect any changes there are the
most likely to warrant additional review.

http://codereview.appspot.com/4929043/diff/5001/lib/Sema/SemaDecl.cpp
File lib/Sema/SemaDecl.cpp (right):

http://codereview.appspot.com/4929043/diff/5001/lib/Sema/SemaDecl.cpp#newcode...
lib/Sema/SemaDecl.cpp:2909: /// hasSimilarParameters - Determine whether the C++
functions Declaration
On 2011/08/24 23:13:06, chandlerc wrote:
> s/functions/function's/

No, it should be plural not singular-possessive because "functions" is referring
to the two FunctionDecl parameters named Declaration and Definition, which refer
to two C++ functions. hasSimilarParameters does not know or care if Declaration
and Definition are FunctionDecls for the same logical C++ function or for two
unrelated functions.

http://codereview.appspot.com/4929043/diff/5001/lib/Sema/SemaDecl.cpp#newcode...
lib/Sema/SemaDecl.cpp:4205: // The rest of the parameters are for calling
ActOnFunctionDeclarator
On 2011/08/24 23:13:06, chandlerc wrote:
> Ugh. Could we stash these in a parameter object, or somehow factor it so that
> they're more clearly managed? That would also help with the confusing naming
of
> 'S' and 'Sc' for example.

There is a generic parameter object in C++/STL/LLVM/Clang? This was one of the
times I was really wishing I could do the equivalent of the Python "def
foo(arg_list): bar(x, *arg_list)". I didn't want to manually define, pack and
unpack a struct just for a static function that is only called in two places in
another single function.

I also agree that S and Sc are confusing... but ran into a naming collision with
this function needing a Sema argument, but then having to add a Scope argument
to pass on to ActOnFunctionDeclarator. Damned overly short type names that have
the same capitalization convention as variables. ;) I'm open to suggestions for
meaningful and not overly verbose alternate names...
> 
> Also, this function definitely needs a doxyment.

Will do.

http://codereview.appspot.com/4929043/diff/5001/lib/Sema/SemaDecl.cpp#newcode...
lib/Sema/SemaDecl.cpp:4244: // Back up the value of the by-reference booleans
On 2011/08/24 23:13:06, chandlerc wrote:
> I have a slight preference to call the function with our own local variables,
> and then to set the output parameters if it succeeds... Makes for nice names
> like "TrialRedeclaration" and "TrialAddToScope"

I considered both ways, and decided to reset them if necessary on the assumption
that DiagnoseInvalidRedeclaration would more often deal with either a typo *or*
mismatched parameter types or cv-qualifiers than with both a typo and a
mismatch.

Also, since there's a lot of side-effect state changes, I'd rather the
consistency of just clean up all of the affected state when there is an error,
rather than having to clean up some state when there is an error and propagate
the rest of the side effects when there isn't an error.

http://codereview.appspot.com/4929043/diff/5001/lib/Sema/SemaDecl.cpp#newcode...
lib/Sema/SemaDecl.cpp:4284: if (TypoWasCorrected)
On 2011/08/24 23:13:06, chandlerc wrote:
> Why not just hoist this into the code that sets TypoWasCorrected to true?

The first part could be hoisted into the code that sets TypoWasCorrected to
true, but the second part would then need an "if (!TypoWasCorrected)" to guard
it so that it can happen whether typo correction was not performed or was
rejected. I didn't go that route as I found it clearer to keep the choice of
which diagnostic to print together instead of having part of it buried in the
logic above (and the FixItHint kept me from merging the two S.Diag call
variants).

> 
> Hmm, at this point in this function's complexity, I think we need to split it
> into several functions. Specifically I'm thinking the loop below to emit notes
> should be a separate function that we can call from the various places where
it
> makes sense, and then we can hoist these diagnostics up into the branches
above,
> and switch them to use early exit, reducing indentation.
> 
> Does that make sense? Separate patches would be fine with me here, and those
> should be fine for post-commit review.

That does make sense... however, looking at the code with fresher eyes, the
typo-correction-specific diagnostics can be folded into the code above where
TypoWasCorrected is set to true without creating another helper function.
Instead of populating NearMatches, that loop could just emit the diagnostics
instead. I'll upload a new patch with the changes I'm talking about once I have
them done along with the comment for DiagnoseInvalidRedeclaration added and
hopefully the parameter list fugliness cleaned up a bit.

http://codereview.appspot.com/4929043/diff/5001/lib/Sema/SemaDecl.cpp#newcode...
lib/Sema/SemaDecl.cpp:4290: S.Diag(NewFD->getLocation(), DiagMsg) << Name <<
NewDC << NewFD->getLocation();
On 2011/08/24 23:13:06, chandlerc wrote:
> 80 columns

Damned three additional characters! ;)

[Kaelyn, 2011-08-30 23:26:57]

Since trying to refactor Sema::ActOnFunctionDeclarator has proved to be far
more time consuming and invasive (lots of state needed throughout the func
spread among multiple variables with far too many order-dependent calls to
other complex functions that depend heavily on side-effects), I've updated
the current fix for DiganoseInvalidRedeclaration typo correction to pack the
extra args needed to call Sema::ActOnFunctionDeclarator into a struct. And
yes, I suspect the run-on sentence complete with tangents is a symptom of
spending so much time digging around in and trying to untangle
Sema::ActOnFunctionDeclarator.

The updated patch is attached, as rietveld is still too lame to send out an
email with a patch or any kind of description when using its upload.py
script to update an existing issue with a new patchset.

Cheers,
Kaelyn

On Tue, Aug 30, 2011 at 4:16 PM, <rikka@google.com> wrote:

>
http://codereview.appspot.com/**4929043/<http://codereview.appspot.com/4929043/>
>

[chandlerc, 2011-08-30 23:29:53]

Ugh. I dunno what the right call is here. This is a kinda horrible hack, if
slightly less horrible than the last iteration... But I don't think anything is
going to fix it short of the refactoring you have a FIXME for... I'd probably
just wait for that refactoring to land, but maybe Doug feels differently.

http://codereview.appspot.com/4929043/diff/13001/lib/Sema/SemaDecl.cpp
File lib/Sema/SemaDecl.cpp (right):

http://codereview.appspot.com/4929043/diff/13001/lib/Sema/SemaDecl.cpp#newcod...
lib/Sema/SemaDecl.cpp:4205: struct AoFDParameterPack {
We need to put this into an anonymous namespace. Also, "ParameterPack" means
something very specific in C++. Maybe: ActOnFDArgs? Yuck. Dunno.

[Kaelyn, 2011-08-31 00:20:29]

On 2011/08/30 23:29:53, chandlerc wrote:
> Ugh. I dunno what the right call is here. This is a kinda horrible hack, if
> slightly less horrible than the last iteration... But I don't think anything
is
> going to fix it short of the refactoring you have a FIXME for... I'd probably
> just wait for that refactoring to land, but maybe Doug feels differently.

I agree this is a horrible hack (and right now feeling like it is only
marginally more horrid than the body of ActOnFunctionDeclarator), but I don't
know when the refactoring might even be close to landing... and I'm starting to
suspect the refactoring may not even help enough to eliminate the need to pass a
bunch of otherwise-unused arguments to DiagnoseInvalidRedeclaration so that it
can check a typo correction. Did just discover that it would be possible to
compute 4 of the 9 extra args to ActOnFunctionDeclarator from within
DiagnoseInvalidRedeclaration...

> 
> http://codereview.appspot.com/4929043/diff/13001/lib/Sema/SemaDecl.cpp
> File lib/Sema/SemaDecl.cpp (right):
> 
>
http://codereview.appspot.com/4929043/diff/13001/lib/Sema/SemaDecl.cpp#newcod...
> lib/Sema/SemaDecl.cpp:4205: struct AoFDParameterPack {
> We need to put this into an anonymous namespace. Also, "ParameterPack" means
> something very specific in C++. Maybe: ActOnFDArgs? Yuck. Dunno.

Done and done.