[cfe-commits] Restart handling of a function declarator if the function name was typo-corrected (issue 4929043)

[rikka at google.com]

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#newcode2909
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#newcode4205
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#newcode4244
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#newcode4284
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#newcode4290
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! ;)

http://codereview.appspot.com/4929043/