<div class="gmail_quote">On Fri, Jan 4, 2013 at 2:19 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Thu, Jan 3, 2013 at 11:18 AM, Christopher Berner <<a href="mailto:cberner@fb.com">cberner@fb.com</a>> wrote:<br>
> I've run into this bug (<a href="http://llvm.org/bugs/show_bug.cgi?id=13517" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=13517</a>), which<br>
> seems to be related to the fixme in SemaType.cpp on line 2691.<br>
<br>
</div>I believe you mean PR12008, not PR13517 -- PR13517 was originally<br>
about a different issue, and appears to have been hijacked for this<br>
bug. There are two patches on PR12008 already, with different<br>
approaches to solving this (neither of which quite works correctly).<br>
Sorry that this fell off the radar, and thanks for looking into it!</blockquote><div><br></div><div>Fixed in r172376.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
> I talked to Chandler Carruth, and he suggested that I look at<br>
> Sema::MergeFunctionDecl. Moving all the logic there (like that fixme seems<br>
> to imply) didn't seem to work, because then functions that have a single<br>
> definition don't get const added to them.<br>
<br>
</div>I would imagine Chandler was intending you'd strip the 'const' off<br>
there, if we find the function to be a redeclaration of a static<br>
member function. That's the approach which Eli's patch takes (see<br>
PR12008 comment#3); one problem with it is that it leads to us<br>
accepting:<br>
<br>
  struct S {<br>
    static constexpr int f();<br>
  };<br>
  constexpr int S::f() const { return 0; }<br>
<br>
... because we don't notice that there's an explicit 'const' in<br>
addition to the implicit one. We currently don't preserve enough<br>
information to detect this (there are spare bits in FunctionDecl in<br>
which we could store an IsConstAsWritten flag if we wanted to go this<br>
way, or we could add qualifier locations to FunctionTypeLoc).<br>
<br>
<br>
The other patch on that bug (see PR12008 comment#4) defers adding the<br>
'const' until we perform redeclaration lookup and determine whether<br>
the method is actually a static data member. (For reasons I don't<br>
recall, it changes the TypeSourceInfo rather than just changing the<br>
type of the function; that seems like a mistake, and whatever problem<br>
it was trying to address should be fixed a different way.) I think<br>
that's a cleaner approach than adding the 'const' then stripping it<br>
off again.<br>
<br>
At the time when I wrote that patch, I was concerned about its<br>
correctness if 'this' is used in a trailing-return-type (see<br>
comment#5) and we defer adding the 'const' until later:<br>
<br>
  struct S {<br>
    constexpr const S *f();<br>
  };<br>
  // Need to know that 'this' has type 'const S*', not just 'S*', here.<br>
  constexpr auto S::f() -> decltype(this) { return this; }<br>
<br>
The mechanism we ended up using for 'this' in trailing-return-types<br>
does not look at the type of the function, so that is not actually an<br>
issue. However, we do mishandle the above case anyway, because we<br>
don't add a 'const' to the 'this' override type for a constexpr<br>
function -- look for the CXXThisScopeRAII object in<br>
Parser::ParseFunctionDeclarator to see where to fix that, if you're<br>
interested.</blockquote><div><br></div><div>Fixed in r172375.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> Instead, I put logic there to remove the errant constness from static<br>
> functions. I've attached a patch. Please let me know if you have any<br>
> comments, as I've never contributed to the Clang codebase before.<br>
<br>
</div>The patch doesn't look quite right in a few ways. You would need to<br>
change the function type itself, not add const to the QualType, and<br>
you shouldn't be changing the type of the old function declaration. I<br>
would suggest you start with one of the two patches on PR12008, update<br>
it to apply to trunk, and fix any residual issues there.<br>
<br>
Thanks!<br>
<span class="HOEnZb"><font color="#888888">Richard<br>
</font></span></blockquote></div><br>