Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2323)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 3 months ago by Kaelyn
Modified:
8 years, 2 months ago
Reviewers:
chandlerc1, dgregor, chandlerc, jediknil
CC:
cfe-commits_cs.uiuc.edu
Base URL:
https://llvm.org/svn/llvm-project/cfe/trunk/
Visibility:
Public.

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.

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+105 lines, -29 lines) Patch
M lib/Sema/SemaDecl.cpp View 1 2 3 6 chunks +101 lines, -28 lines 0 comments Download
M test/SemaCXX/function-redecl.cpp View 1 2 chunks +4 lines, -1 line 0 comments Download

Messages

Total messages: 12
Kaelyn
8 years, 3 months ago (2011-08-19 20:47:05 UTC) #1
jediknil_belkadan.com
I'm not a fan of emitting errors for code the user didn't write, but I'd ...
8 years, 3 months ago (2011-08-21 05:34:17 UTC) #2
Kaelyn
I agree that a fixit for const mismatches would be nice too, but the focus ...
8 years, 3 months ago (2011-08-22 16:30:45 UTC) #3
Kaelyn
8 years, 2 months ago (2011-08-24 19:57:58 UTC) #4
Kaelyn
Rietveld's upload.py seems to not have included the patch or description when I used it ...
8 years, 2 months ago (2011-08-24 20:07:16 UTC) #5
chandlerc
Thanks for working on this. I wonder if we need to generalize this whole pattern ...
8 years, 2 months ago (2011-08-24 23:13:06 UTC) #6
Kaelyn
Chandler, I'd like to get any additional comments you have about the function parameters before ...
8 years, 2 months ago (2011-08-25 00:14:54 UTC) #7
Kaelyn
8 years, 2 months ago (2011-08-30 23:16:34 UTC) #8
Kaelyn
Since trying to refactor Sema::ActOnFunctionDeclarator has proved to be far more time consuming and invasive ...
8 years, 2 months ago (2011-08-30 23:26:57 UTC) #9
chandlerc
Ugh. I dunno what the right call is here. This is a kinda horrible hack, ...
8 years, 2 months ago (2011-08-30 23:29:53 UTC) #10
Kaelyn
8 years, 2 months ago (2011-08-31 00:20:15 UTC) #11
Kaelyn
8 years, 2 months ago (2011-08-31 00:20:29 UTC) #12
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b