On Fri, Jan 4, 2013 at 2:19 PM, Richard Smith <[email protected]> wrote:
> On Thu, Jan 3, 2013 at 11:18 AM, Christopher Berner <[email protected]> > 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 >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
