[PATCH] Add more information when displaying a "read-only variable is not assignable" error

[Richard Smith]

On Tue, Jul 15, 2014 at 9:24 AM, David Blaikie <dblaikie at gmail.com> wrote:

> >>! In D4479#7, @rtrieu wrote:
> >> Also, would it be better for the diagnostic to point to the "const" in
> the function rather than the "f"?
> >
> > Probably, but the location of the const on a function does not appear to
> be available.
>
> Mr. Smith - any thoughts on whether this should be available? Or is it
> just not feasible due to AST size costs? In any case, if it's not
> available, that's OK for this commit & maybe leave a FIXME in the test and
> in the source suggesting that it could be improved if we get that source
> fidelity in the AST some day.


It *should* be, but it's a historical deficiency in TypeLoc that we do not
model the locations of cv-qualifiers.


> >>! In D4479#6, @rtrieu wrote:
> > ```
> > $ cat c.cc
> > struct foo {
> >   int i;
> >   void f() const;
> > };
> > void foo::f() cont {
> >   i = 3;
> > }
> > $ clang c.cc
> > c.cc:6:5: error: read-only variable is not assignable
> >   i = 3;
> >   ~ ^
> > c.cc:5:11: note: method 'f' is declared const here
> > void foo::f() const {
> > ~~~~~~~~~~^~~~~~~~~
> > 1 error generated.
> >
> > ```
> > Note will point to function definition.
>
> Great (not sure about the "declared const here" phrasing, though -
> technically true (a definition is a declaration, though most people think
> of the first declaration as "the declaration") and I can't think of much
> better, though)


I think I'd prefer to see:

error: cannot assign to data member 'i' within const member function
note: member function 'foo::f' is declared const here

>
> >
> >
> > ```
> > $ cat d.cc
> > typedef const int X;
> > void test(X x) {
> >   x = 5;
> > }
> > $ clang d.cc
> > d.cc:3:5: error: read-only variable is not assignable
> >   x = 5;
> >   ~ ^
> > d.cc:2:13: note: variable 'x' is declared here with type 'X' (aka 'const
> int') which is const
> > void test(X x) {
> >           ~~^
> > 1 error generated.
> >
> > ```
> > No problems with typedefs.
> >
> > I think the note should explicitly point out the const qualifier,
> otherwise the user will see "note: variable 'x' has type 'y'" which isn't
> very helpful.
>
> Depends if the 'y' has the word 'const' in it, in which case it's fairly
> helpful. Though that's why I was suggesting possibly repurposing the type
> diffing code to highlight the "const"-ness.


I'd prefer:

error: cannot assign to variable 'x' with const-qualified type 'y'
note: declared here


> > Perhaps "note: variable "x" is declared here with type 'const int';
> const qualifier prevents assignment" would be better?
>
> That sounds pretty good to me. Would have to see it in a few more examples
> (like the const this pointer case, etc) to see how the phrasing generalizes.
>

Generally, I think we should put enough information in the error that the
user can get a general sense of what's wrong before they read the notes, so
I'd like to see the headline information moved from the notes to the errors
in this case.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140715/b066405b/attachment.html>