|
|
|
Created:
14 years, 4 months ago by Kaelyn Modified:
14 years, 4 months ago CC:
cfe-commits_cs.uiuc.edu Base URL:
https://llvm.org/svn/llvm-project/cfe/trunk/ Visibility:
Public. |
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.
Patch Set 1 #Patch Set 2 : Only accept a typo correction and redo ActOnFunctionDeclarator if doing so doesn't trigger errors #
Total comments: 10
Patch Set 3 : Pack the extra args for calling ActOnFunctionDeclarator into a struct, and add some comments #
Total comments: 1
Patch Set 4 : Pack the extra args for calling ActOnFunctionDeclarator into a struct, and add some comments #
Total messages: 12
|
Kaelyn
|
14 years, 4 months ago (2011年08月19日 20:47:05 UTC) #1 | |||||||||||||||||||||||
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
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 > >
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/> >
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
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! ;)
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/> >
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.
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.