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

[Richard Smith]

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!

> 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.

> 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