Make DiagnoseInvalidRedeclaration state when a member decl does not match due to "const" mismatch

[eli.friedman_gmail.com, 2011-09-30 23:39:37]

On Fri, Sep 30, 2011 at 4:12 PM,  <rikka@google.com> wrote:
> Reviewers: chandlerc,
>
>
>
> Please review this at http://codereview.appspot.com/5167048/
>
> Affected files:
>  M include/clang/Basic/DiagnosticSemaKinds.td
>  M lib/Sema/SemaDecl.cpp
>  M test/SemaCXX/function-redecl.cpp
>  M test/SemaCXX/nested-name-spec.cpp

I don't really like the wording "member declaration has const
keyword"; there is no guarantee that the note points anywhere near the
keyword "const", and you've lost the "nearly matches" part of the
original diagnostic.  Maybe something more like "member declaration
does not match because it is const qualified"?

The code looks fine.

-Eli

[Kaelyn, 2011-10-03 16:43:39]

On Fri, Sep 30, 2011 at 4:39 PM, Eli Friedman <eli.friedman@gmail.com>wrote:

> On Fri, Sep 30, 2011 at 4:12 PM,  <rikka@google.com> wrote:
> > Reviewers: chandlerc,
> >
> >
> >
> > Please review this at http://codereview.appspot.com/5167048/
> >
> > Affected files:
> >  M include/clang/Basic/DiagnosticSemaKinds.td
> >  M lib/Sema/SemaDecl.cpp
> >  M test/SemaCXX/function-redecl.cpp
> >  M test/SemaCXX/nested-name-spec.cpp
>
> I don't really like the wording "member declaration has const
> keyword"; there is no guarantee that the note points anywhere near the
> keyword "const", and you've lost the "nearly matches" part of the
> original diagnostic.  Maybe something more like "member declaration
> does not match because it is const qualified"?
>

Yeah, I was having trouble coming up with wording that would work both when
the declaration being diagnosed doesn't have a "const" but one of the
matched decls does, and when the matched decl doesn't but the decl being
diagnosed does. In particular I was trying to avoid having those two cases
being two separate Diag statements under two branches of an if statement.

Cheers,
Kaelyn

>
> The code looks fine.
>
> -Eli
>

[chandlerc1, 2011-10-03 16:53:41]

On Mon, Oct 3, 2011 at 9:43 AM, Kaelyn Uhrain <rikka@google.com> wrote:

> Yeah, I was having trouble coming up with wording that would work both when
> the declaration being diagnosed doesn't have a "const" but one of the
> matched decls does, and when the matched decl doesn't but the decl being
> diagnosed does. In particular I was trying to avoid having those two cases
> being two separate Diag statements under two branches of an if statement.


What about (based on Eli's example): "member declaration does not match
because it %select{is|is not}N const qualified"

It seems good to have the message explicitly indicate in which direction the
error was made, and we can use select to avoid over complex emission code...

[dblaikie_gmail.com, 2011-10-03 16:54:09]

On Mon, Oct 3, 2011 at 9:43 AM, Kaelyn Uhrain <rikka@google.com> wrote:

> On Fri, Sep 30, 2011 at 4:39 PM, Eli Friedman <eli.friedman@gmail.com>wrote:
>
>> On Fri, Sep 30, 2011 at 4:12 PM,  <rikka@google.com> wrote:
>> > Reviewers: chandlerc,
>> >
>> >
>> >
>> > Please review this at http://codereview.appspot.com/5167048/
>> >
>> > Affected files:
>> >  M include/clang/Basic/DiagnosticSemaKinds.td
>> >  M lib/Sema/SemaDecl.cpp
>> >  M test/SemaCXX/function-redecl.cpp
>> >  M test/SemaCXX/nested-name-spec.cpp
>>
>> I don't really like the wording "member declaration has const
>> keyword"; there is no guarantee that the note points anywhere near the
>> keyword "const", and you've lost the "nearly matches" part of the
>> original diagnostic.  Maybe something more like "member declaration
>> does not match because it is const qualified"?
>>
>
> Yeah, I was having trouble coming up with wording that would work both when
> the declaration being diagnosed doesn't have a "const" but one of the
> matched decls does, and when the matched decl doesn't but the decl being
> diagnosed does. In particular I was trying to avoid having those two cases
> being two separate Diag statements under two branches of an if statement.
>

Any particular reason you were trying to avoid that? I expect it'd probably
make for better diagnostic text (as Eli was suggesting - though admittedly
we haven't quite arrived at the ideal text yet) but I expect it'd also be
nice to add fixits too & those would differ between the two cases (if we
chose a consistent approach - eg: always assume the original declaration is
correct so issue a fixit to fix the definition (by adding or removing
const)).

- David

[Kaelyn, 2011-10-03 17:02:43]

On Mon, Oct 3, 2011 at 9:53 AM, Chandler Carruth <chandlerc@google.com>wrote:

> On Mon, Oct 3, 2011 at 9:43 AM, Kaelyn Uhrain <rikka@google.com> wrote:
>
>> Yeah, I was having trouble coming up with wording that would work both
>> when the declaration being diagnosed doesn't have a "const" but one of the
>> matched decls does, and when the matched decl doesn't but the decl being
>> diagnosed does. In particular I was trying to avoid having those two cases
>> being two separate Diag statements under two branches of an if statement.
>
>
> What about (based on Eli's example): "member declaration does not match
> because it %select{is|is not}N const qualified"
>
> It seems good to have the message explicitly indicate in which direction
> the error was made, and we can use select to avoid over complex emission
> code...
>

Yup, already using a %select in the diagnostic to be able to explicitly
indicate the direction of the mismatch. My comment wasn't directed at Eli's
example but more a general comment that I trying to come up with a message
for which I could use %select. If folks are fine with the wording Eli
suggested then I'll use that and submit the patch. ;)

[Kaelyn, 2011-10-10 17:39:56]

One week and no additional comments... I'm going to submit this patch now,
incorporating the wording change suggested by Eli ("member declaration does
not match because it %select{is|is not}0 const qualified" instead of "member
declaration %select{has|lacks}0 const keyword").

Cheers,
Kaelyn

On Mon, Oct 3, 2011 at 10:02 AM, Kaelyn Uhrain <rikka@google.com> wrote:

> On Mon, Oct 3, 2011 at 9:53 AM, Chandler Carruth <chandlerc@google.com>wrote:
>
>> On Mon, Oct 3, 2011 at 9:43 AM, Kaelyn Uhrain <rikka@google.com> wrote:
>>
>>> Yeah, I was having trouble coming up with wording that would work both
>>> when the declaration being diagnosed doesn't have a "const" but one of the
>>> matched decls does, and when the matched decl doesn't but the decl being
>>> diagnosed does. In particular I was trying to avoid having those two cases
>>> being two separate Diag statements under two branches of an if statement.
>>
>>
>> What about (based on Eli's example): "member declaration does not match
>> because it %select{is|is not}N const qualified"
>>
>> It seems good to have the message explicitly indicate in which direction
>> the error was made, and we can use select to avoid over complex emission
>> code...
>>
>
> Yup, already using a %select in the diagnostic to be able to explicitly
> indicate the direction of the mismatch. My comment wasn't directed at Eli's
> example but more a general comment that I trying to come up with a message
> for which I could use %select. If folks are fine with the wording Eli
> suggested then I'll use that and submit the patch. ;)
>