<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jul 15, 2014 at 9:24 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">>>! In D4479#7, @rtrieu wrote:<br>
>> Also, would it be better for the diagnostic to point to the "const" in the function rather than the "f"?<br>
><br>
> Probably, but the location of the const on a function does not appear to be available.<br>
<br>
</div>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.</blockquote>
<div><br></div><div>It *should* be, but it's a historical deficiency in TypeLoc that we do not model the locations of cv-qualifiers.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="">
>>! In D4479#6, @rtrieu wrote:<br>
> ```<br>
> $ cat c.cc<br>
> struct foo {<br>
>   int i;<br>
>   void f() const;<br>
> };<br>
> void foo::f() cont {<br>
>   i = 3;<br>
> }<br>
> $ clang c.cc<br>
> c.cc:6:5: error: read-only variable is not assignable<br>
>   i = 3;<br>
>   ~ ^<br>
> c.cc:5:11: note: method 'f' is declared const here<br>
> void foo::f() const {<br>
> ~~~~~~~~~~^~~~~~~~~<br>
> 1 error generated.<br>
><br>
> ```<br>
> Note will point to function definition.<br>
<br>
</div>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)</blockquote>
<div><br></div><div>I think I'd prefer to see:</div><div><br></div><div>error: cannot assign to data member 'i' within const member function</div><div>note: member function 'foo::f' is declared const here</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
><br>
><br>
><br>
> ```<br>
> $ cat d.cc<br>
> typedef const int X;<br>
> void test(X x) {<br>
>   x = 5;<br>
> }<br>
> $ clang d.cc<br>
> d.cc:3:5: error: read-only variable is not assignable<br>
>   x = 5;<br>
>   ~ ^<br>
> d.cc:2:13: note: variable 'x' is declared here with type 'X' (aka 'const int') which is const<br>
> void test(X x) {<br>
>           ~~^<br>
> 1 error generated.<br>
><br>
> ```<br>
> No problems with typedefs.<br>
><br>
> 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.<br>
<br>
</div>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.</blockquote>
<div><br></div><div>I'd prefer:</div><div><br></div><div>error: cannot assign to variable 'x' with const-qualified type 'y'</div><div>note: declared here</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="">
> Perhaps "note: variable "x" is declared here with type 'const int'; const qualifier prevents assignment" would be better?<br>
<br>
</div>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.<br></blockquote><div><br></div><div>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.</div>
</div></div></div>