[PATCH] D37308: Interface class with uuid base record

[Erich Keane via Phabricator via cfe-commits]

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