erichkeane added a comment.
Also, Craig mentioned to me that naming rules require static fucntions to start
with a lower case letter (not uppercase). Additionally, Variables (like
'result') need to start with a capital letter.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:2388
+
+ return Uuid && Uuid->getGuid() =="00000000-0000-0000-C000-000000000046" &&
+ RD->isStruct() && RD->getName() == "IUnknown" && RD->isEmpty() &&
----------------
Up to @aaron.ballman, but moving the getGuid string check (a massive
string-diff) to 2nd defeats the purpose of moving UUID up front, which is to
put the 'easy' things first. IsTU, isStruct, and isEmpty are likely better.
Additionally, I'd mentioned it before, this still doesn't check to make sure
that the IUnknown doesn't inherit from anywhere, does it? MSVC also will error
if IUnknown's definition includes an inherited type.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:2393
+
+/// \brief Test if RD or its inherited bases is an IUnknow type.
+static bool IsOrInheritsFromIUnknown(const CXXRecordDecl *RD) {
----------------
Still a misspelled comment.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:2395
+static bool IsOrInheritsFromIUnknown(const CXXRecordDecl *RD) {
+ bool result = IsIUnknownType(RD);
+ for (const auto *I = RD->bases_begin(), *E = RD->bases_end(); I != E; ++I) {
----------------
Since you know the value of this here, there is no reason to waste time with
the below loop. Return immediately.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:2396
+ bool result = IsIUnknownType(RD);
+ for (const auto *I = RD->bases_begin(), *E = RD->bases_end(); I != E; ++I) {
+ const CXXRecordDecl *BB = I->getType()->getAsCXXRecordDecl();
----------------
This should use a range-based for.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:2397
+ for (const auto *I = RD->bases_begin(), *E = RD->bases_end(); I != E; ++I) {
+ const CXXRecordDecl *BB = I->getType()->getAsCXXRecordDecl();
+ return result || IsOrInheritsFromIUnknown(BB);
----------------
What does "BB" stand for? Please use a descriptive name.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:2398
+ const CXXRecordDecl *BB = I->getType()->getAsCXXRecordDecl();
+ return result || IsOrInheritsFromIUnknown(BB);
+ }
----------------
Two big issues here:
1- This will never allow this loop to hit the second iteration, which it
absolutely needs to do.
2- Again, you're in a situation where you only need to continue the execution
of this function in certain cases.
https://reviews.llvm.org/D37308
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
