[cfe-commits] r132572 - in /cfe/trunk: include/clang/AST/Type.h include/clang/Sema/Sema.h lib/Sema/SemaLookup.cpp
Chandler Carruth
chandlerc at google.com
Fri Jun 3 13:36:35 PDT 2011
Sorry for duplicate email. Hit the wrong button...
On Fri, Jun 3, 2011 at 11:36 AM, Sean Hunt <scshunt at csclub.uwaterloo.ca>wrote:
>
> + static unsigned getHashValue (const clang::SpecialMemberID SMI) {
> + // Vary higher bits of the pointer for hashing. Attempt to match the
> + // bit-field representation to reduce masking if the optimizer is
> awake.
> + // Note that the LLVM optimizer sleeps through this one.
> + return (uintptr_t)SMI.D ^
> + ((SMI.SM << 24) + (SMI.ConstArg << 27) + (SMI.VolatileArg << 28) +
> + (SMI.RValueThis << 29) + (SMI.ConstThis << 30) +
> + (SMI.VolatileThis << 31));
>
I worry about the person who adds a new bitfield to the type, but fails to
update this function. Is there a way to both avoid specifying each of these
fields, and their offsets? My current theory would be to memcpy from the
offset of the first bitfield, and then shift into high bits based on the
offset of a dummy final field.
> + }
> + static bool isEqual(const clang::SpecialMemberID LHS,
> + const clang::SpecialMemberID RHS) {
> + return LHS.D == RHS.D && LHS.SM == RHS.SM && LHS.ConstArg ==
> RHS.ConstArg &&
> + LHS.VolatileArg == RHS.VolatileArg &&
> + LHS.RValueThis == RHS.RValueThis && LHS.ConstThis ==
> RHS.ConstThis &&
> + LHS.VolatileThis == RHS.VolatileThis;
>
memcmp? Actually, why can't we just use LHS == RHS and let the compiler do
the work for us? Again listing these down here seems really unfortunate for
a future maintainer.
If we have to manually enumerate the members, can we do it in an
operator==() in the struct so that its more clear it has to be updated when
the struct is updated?
+ }
> +};
> +} // namespace llvm
> +
> +namespace clang {
> /// Sema - This implements semantic analysis and AST building for C.
> class Sema {
> Sema(const Sema&); // DO NOT IMPLEMENT
> @@ -600,6 +646,47 @@
> /// A stack of expression evaluation contexts.
> llvm::SmallVector<ExpressionEvaluationContextRecord, 8> ExprEvalContexts;
>
> + /// SpecialMemberOverloadResult - The overloading result for a special
> member
> + /// function.
> + ///
> + /// This is basically a wrapper around PointerIntPair. The lowest bit of
> the
> + /// integer is used to determine whether we have a parameter
> qualification
> + /// match, the second-lowest is whether we had success in resolving the
> + /// overload to a unique non-deleted function.
> + ///
> + /// The ConstParamMatch bit represents whether, when looking up a copy
> + /// constructor or assignment operator, we found a potential copy
> + /// constructor/assignment operator whose first parameter is
> const-qualified.
>
You've documented one of these bits twice. The first paragraph mentions the
lowest and second lowest bit. The second paragraph talks about "The
ConstParamMatch bit" which isn't defined, but seems below to correspond to
the second lowest bit.
> + /// This is used for determining parameter types of other objects and is
> + /// utterly meaningless on other types of special members.
>
This comment doesn't make sense for me....
> + class SpecialMemberOverloadResult {
> + llvm::PointerIntPair<CXXMethodDecl*, 2> Pair;
> + public:
> + SpecialMemberOverloadResult(CXXMethodDecl *MD, bool Success,
> + bool ConstParamMatch)
> + : Pair(MD, Success | ConstParamMatch << 1)
> + {}
> + SpecialMemberOverloadResult() {}
> +
> + CXXMethodDecl *getMethod() const { return Pair.getPointer(); }
> + void setMethod(CXXMethodDecl *MD) { Pair.setPointer(MD); }
> +
> + bool hasSuccess() const { return Pair.getInt() & 0x1; }
> + void setSuccess(bool B) { Pair.setInt(B | hasConstParamMatch() << 1);
> }
> +
> + bool hasConstParamMatch() const { return Pair.getInt() & 0x2; }
> + void setConstParamMatch(bool B) { Pair.setInt(B << 1 | hasSuccess());
> }
> + };
> +
> + /// \brief A cache of special member function overload resolution
> results
> + /// for C++ records.
> + ///
> + /// In C++, special member functions of records (such as the copy
> constructor)
> + /// are used a lot. As a result, we cache the lookups here so as to make
> the
> + /// lookups far easier to perform.
>
I'm not sure what this comment is adding past the brief comment...
> + llvm::DenseMap<SpecialMemberID, SpecialMemberOverloadResult>
> + SpecialMemberCache;
> +
> /// \brief Whether the code handled by Sema should be considered a
> /// complete translation unit or not.
> ///
> @@ -1594,6 +1681,14 @@
> private:
> bool CppLookupName(LookupResult &R, Scope *S);
>
> + SpecialMemberOverloadResult LookupSpecialMember(CXXRecordDecl *D,
> + CXXSpecialMember SM,
> + bool ConstArg,
> + bool VolatileArg,
> + bool RValueThis,
> + bool ConstThis,
> + bool VolatileThis);
> +
> public:
> /// \brief Look up a name, looking for a single declaration. Return
> /// null if the results were absent, ambiguous, or overloaded.
>
> Modified: cfe/trunk/lib/Sema/SemaLookup.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaLookup.cpp?rev=132572&r1=132571&r2=132572&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaLookup.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaLookup.cpp Fri Jun 3 13:36:49 2011
> @@ -14,6 +14,7 @@
> #include "clang/Sema/Sema.h"
> #include "clang/Sema/SemaInternal.h"
> #include "clang/Sema/Lookup.h"
> +#include "clang/Sema/Overload.h"
> #include "clang/Sema/DeclSpec.h"
> #include "clang/Sema/Scope.h"
> #include "clang/Sema/ScopeInfo.h"
> @@ -2136,6 +2137,51 @@
> }
> }
>
> +Sema::SpecialMemberOverloadResult Sema::LookupSpecialMember(CXXRecordDecl
> *D,
> +
> CXXSpecialMember SM,
> + bool ConstArg,
> + bool
> VolatileArg,
> + bool
> RValueThis,
> + bool
> ConstThis,
> + bool
> VolatileThis) {
> + D = D->getDefinition();
> + assert((D && !D->isBeingDefined()) &&
> + "doing special member lookup into record that isn't fully
> complete");
> + if (RValueThis || ConstThis || VolatileThis)
> + assert((SM == CXXCopyAssignment || SM == CXXMoveAssignment) &&
> + "constructors and destructors always have unqualified lvalue
> this");
> + if (ConstArg || VolatileArg)
> + assert((SM != CXXDefaultConstructor && SM != CXXDestructor) &&
> + "parameter-less special members can't have qualified
> arguments");
> +
> + // Check the cache for this member
> + SpecialMemberID ID = {D, SM, ConstArg, VolatileArg, RValueThis,
> ConstThis,
> + VolatileThis};
> + SpecialMemberOverloadResult Blank;
> + llvm::DenseMap<SpecialMemberID, SpecialMemberOverloadResult>::iterator
> It;
> + bool New;
> +
> + llvm::tie(It, New) = SpecialMemberCache.insert(std::make_pair(ID,
> Blank));
> + SpecialMemberOverloadResult &Result = It->second;
>
This seems like the exact pattern that FoldingSet was designed to address...
> +
> + // This was already cached
> + if (!New)
> + return Result;
> +
> + if (SM == CXXDestructor) {
> + if (!D->hasDeclaredDestructor())
> + DeclareImplicitDestructor(D);
> + CXXDestructorDecl *DD = D->getDestructor();
> + assert(DD && "record without a destructor");
> + Result.setMethod(DD);
> + Result.setSuccess(DD->isDeleted());
> + Result.setConstParamMatch(false);
> + return Result;
> + }
> +
> + llvm_unreachable("haven't implemented this for non-destructors yet");
> +}
> +
> /// \brief Look up the constructors for the given class.
> DeclContext::lookup_result Sema::LookupConstructors(CXXRecordDecl *Class)
> {
> // If the copy constructor has not yet been declared, do so now.
> @@ -2153,17 +2199,13 @@
>
> /// \brief Look for the destructor of the given class.
> ///
> -/// During semantic analysis, this routine should be used in lieu of
> -/// CXXRecordDecl::getDestructor().
> +/// The destructor will be declared if necessary.
> ///
> /// \returns The destructor for this class.
> CXXDestructorDecl *Sema::LookupDestructor(CXXRecordDecl *Class) {
> - // If the destructor has not yet been declared, do so now.
> - if (CanDeclareSpecialMemberFunction(Context, Class) &&
> - !Class->hasDeclaredDestructor())
> - DeclareImplicitDestructor(Class);
> -
> - return Class->getDestructor();
> + return cast<CXXDestructorDecl>(LookupSpecialMember(Class, CXXDestructor,
> + false, false, false,
> + false,
> false).getMethod());
> }
>
> void ADLResult::insert(NamedDecl *New) {
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110603/aa926e39/attachment.html>
More information about the cfe-commits
mailing list