[cfe-commits] Fix compilation error with static constexpr member functions

[Richard Smith]

On Fri, Jan 4, 2013 at 2:19 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Thu, Jan 3, 2013 at 11:18 AM, Christopher Berner <cberner at fb.com>
> wrote:
> > I've run into this bug (http://llvm.org/bugs/show_bug.cgi?id=13517),
> which
> > seems to be related to the fixme in SemaType.cpp on line 2691.
>
> I believe you mean PR12008, not PR13517 -- PR13517 was originally
> about a different issue, and appears to have been hijacked for this
> bug. There are two patches on PR12008 already, with different
> approaches to solving this (neither of which quite works correctly).
> Sorry that this fell off the radar, and thanks for looking into it!


Fixed in r172376.

> I talked to Chandler Carruth, and he suggested that I look at
> > Sema::MergeFunctionDecl. Moving all the logic there (like that fixme
> seems
> > to imply) didn't seem to work, because then functions that have a single
> > definition don't get const added to them.
>
> I would imagine Chandler was intending you'd strip the 'const' off
> there, if we find the function to be a redeclaration of a static
> member function. That's the approach which Eli's patch takes (see
> PR12008 comment#3); one problem with it is that it leads to us
> accepting:
>
>   struct S {
>     static constexpr int f();
>   };
>   constexpr int S::f() const { return 0; }
>
> ... because we don't notice that there's an explicit 'const' in
> addition to the implicit one. We currently don't preserve enough
> information to detect this (there are spare bits in FunctionDecl in
> which we could store an IsConstAsWritten flag if we wanted to go this
> way, or we could add qualifier locations to FunctionTypeLoc).
>
>
> The other patch on that bug (see PR12008 comment#4) defers adding the
> 'const' until we perform redeclaration lookup and determine whether
> the method is actually a static data member. (For reasons I don't
> recall, it changes the TypeSourceInfo rather than just changing the
> type of the function; that seems like a mistake, and whatever problem
> it was trying to address should be fixed a different way.) I think
> that's a cleaner approach than adding the 'const' then stripping it
> off again.
>
> At the time when I wrote that patch, I was concerned about its
> correctness if 'this' is used in a trailing-return-type (see
> comment#5) and we defer adding the 'const' until later:
>
>   struct S {
>     constexpr const S *f();
>   };
>   // Need to know that 'this' has type 'const S*', not just 'S*', here.
>   constexpr auto S::f() -> decltype(this) { return this; }
>
> The mechanism we ended up using for 'this' in trailing-return-types
> does not look at the type of the function, so that is not actually an
> issue. However, we do mishandle the above case anyway, because we
> don't add a 'const' to the 'this' override type for a constexpr
> function -- look for the CXXThisScopeRAII object in
> Parser::ParseFunctionDeclarator to see where to fix that, if you're
> interested.


Fixed in r172375.


> > Instead, I put logic there to remove the errant constness from static
> > functions. I've attached a patch. Please let me know if you have any
> > comments, as I've never contributed to the Clang codebase before.
>
> The patch doesn't look quite right in a few ways. You would need to
> change the function type itself, not add const to the QualType, and
> you shouldn't be changing the type of the old function declaration. I
> would suggest you start with one of the two patches on PR12008, update
> it to apply to trunk, and fix any residual issues there.
>
> Thanks!
> Richard
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130113/63d7aa29/attachment.html>