Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter

[shafik]

Summary

Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter.

Both calling sites of the sites were incorrectly calculating the qualtype as the underlying type unconditionally irrespective of whether the enum was unscoped or scoped

[davide]

packages/Python/lldbsuite/test/expression_command/radar_43822994/TestScopedEnumType.py

42 ↗

(On Diff #172244)

This clearly looks like can be an inline test (and probably would make the thing more readable, IMHO).

[teemperor]

This looks nice, just added some minor comments below. Otherwise LGTM after Davide's point is addressed.

include/lldb/Symbol/ClangASTContext.h

909 ↗

(On Diff #172244)

Can we pass enum_type as const ref?

source/Symbol/ClangASTContext.cpp

8851 ↗

(On Diff #172244)

(Same as above) Can we have enum_type as a const ref?

[zturner]

This function is called in SymbolFile/NativePDB as well.

[clayborg]

LGTM. lldbassert to check that AST in "enum_type" is the same as "this" since we now can after switching to CompilerType as arg instead of opaque clang type pointer.

include/lldb/Symbol/ClangASTContext.h

909 ↗

(On Diff #172244)

CompilerType instances are two pointers. Pretty cheap to copy.

source/Symbol/ClangASTContext.cpp

8851 ↗

(On Diff #172244)

CompilerType instances are two pointers. Pretty cheap to copy.

8853 ↗

(On Diff #172244)

lldbassert that the AST in "enum_type" is the same as "this"?

[shafik]

@zturner I don't see AddEnumerationValueToEnumerationType being called in SymbolFile/NativePDB.cpp https://github.com/llvm-mirror/lldb/search?q=AddEnumerationValueToEnumerationType&unscoped_q=AddEnumerationValueToEnumerationType

[zturner]

I meant the folder. It’s the first result in your search results. Just
curious, why does your build successfully complete without fixing that
instance?

[shafik]

@zturner I was out of date when I posted this diff but I have that file updated locally and it will show up when I update the diff.

[jingham]

Looking at the extant API's we do seem to prefer "const CompilerType &" over "const CompilerType" so it seems more consistent to choose that. Other than that, this looks fine to me.

include/lldb/Symbol/ClangASTContext.h

909 ↗

(On Diff #172244)

We're not entirely consistent, but a quick glance through headers show us almost always choosing to pass "const CompilerType &" over "const CompilerType".

packages/Python/lldbsuite/test/expression_command/radar_43822994/TestScopedEnumType.py

42 ↗

(On Diff #172244)

The form of test to use is up to the test writer, seems to me. This test is not at all hard to read.

[shafik]

Made changes based on comments.

[shafik]

@jingham @zturner @teemperor @clayborg I believe I have addressed all the comments.

include/lldb/Symbol/ClangASTContext.h

909 ↗

(On Diff #172244)

That was a good catch, thank you!

[zturner]

include/lldb/Symbol/ClangASTContext.h

909 ↗

(On Diff #172244)

Definitely passing by const value doesn't make sense, but I also think passing by value is what we should do going forward. It doesn't necessarily have to be in this patch, but I don't see any reason to pass by reference in general, and it just adds an extra level of pointer indirection for no real benefit IMO.

[jingham]

Looks good to me.

[shafik]

@zturner I would not be against discussing using pass by value for small objects going forward. I don't currently have a good feeling for at what sizes/data types the right trade-off is at though.

lldb/trunk/include/lldb/Symbol/ClangASTContext.h

#define LLDB_INVALID_DECL_LEVEL UINT32_MAX
   static bool StartTagDeclarationDefinition(const CompilerType &type);
 
   static bool CompleteTagDeclarationDefinition(const CompilerType &type);
 
   //----------------------------------------------------------------------
   // Modifying Enumeration types
   //----------------------------------------------------------------------
   clang::EnumConstantDecl *AddEnumerationValueToEnumerationType(
-      lldb::opaque_compiler_type_t type,
-      const CompilerType &enumerator_qual_type, const Declaration &decl,
-      const char *name, int64_t enum_value, uint32_t enum_value_bit_size);
+      const CompilerType &enum_type, const Declaration &decl, const char *name,
+      int64_t enum_value, uint32_t enum_value_bit_size);
 
   CompilerType GetEnumerationIntegerType(lldb::opaque_compiler_type_t type);
 
   //------------------------------------------------------------------
   // Pointers & References
   //------------------------------------------------------------------
 
   // Call this function using the class type when you want to make a member

lldb/trunk/packages/Python/lldbsuite/test/expression_command/radar_43822994/Makefile

+LEVEL = ../../make
+
+CXX_SOURCES := main.cpp
+CXXFLAGS += -std=c++11
+
+include $(LEVEL)/Makefile.rules

lldb/trunk/packages/Python/lldbsuite/test/expression_command/radar_43822994/TestScopedEnumType.py

+from __future__ import print_function
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class ExprXValuePrintingTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test(self):
+        self.build()
+
+        self.main_source = "main.cpp"
+        self.main_source_spec = lldb.SBFileSpec(self.main_source)
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+                                          '// Set break point at this line.', self.main_source_spec)
+        frame = thread.GetFrameAtIndex(0)
+
+        self.expect("expr f == Foo::FooBar",
+                substrs=['(bool) $0 = true'])
+
+        value = frame.EvaluateExpression("f == Foo::FooBar")
+        self.assertTrue(value.IsValid())
+        self.assertTrue(value.GetError().Success())
+        self.assertEqual(value.GetValueAsUnsigned(), 1)
+
+        value = frame.EvaluateExpression("b == BarBar")
+        self.assertTrue(value.IsValid())
+        self.assertTrue(value.GetError().Success())
+        self.assertEqual(value.GetValueAsUnsigned(), 1)
+
+        ## b is not a Foo
+        value = frame.EvaluateExpression("b == Foo::FooBar")
+        self.assertTrue(value.IsValid())
+        self.assertFalse(value.GetError().Success())
+
+        ## integral is not implicitly convertible to a scoped enum
+        value = frame.EvaluateExpression("1 == Foo::FooBar")
+        self.assertTrue(value.IsValid())
+        self.assertFalse(value.GetError().Success())

lldb/trunk/packages/Python/lldbsuite/test/expression_command/radar_43822994/main.cpp

+enum class Foo {
+  FooBar = 42
+};
+
+enum Bar {
+    BarBar = 3,
+    BarBarBar = 42
+};
+
+int main(int argc, const char **argv) {
+  Foo f = Foo::FooBar;
+  Bar b = BarBar;
+  bool b1 = f == Foo::FooBar;
+  bool b2 = b == BarBar;
+  return 0; // Set break point at this line.
+}

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

if (tag == DW_TAG_enumerator) {
             case DW_AT_sibling:
               break;
             }
           }
         }
 
         if (name && name[0] && got_value) {
           m_ast.AddEnumerationValueToEnumerationType(
-              clang_type.GetOpaqueQualType(),
-              m_ast.GetEnumerationIntegerType(clang_type.GetOpaqueQualType()),
-              decl, name, enum_value, enumerator_byte_size * 8);
+              clang_type, decl, name, enum_value, enumerator_byte_size * 8);
           ++enumerators_added;
         }
       }
     }
   }
   return enumerators_added;
 }
 

lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp

Error UdtRecordCompleter::visitKnownMember(CVMemberRecord &cvr,
   ClangASTContext &clang = m_symbol_file.GetASTContext();
 
   Declaration decl;
   llvm::StringRef name = DropNameScope(enumerator.getName());
   lldbassert(m_uid.tag() == PDB_SymType::Enum);
   TypeSP underlying_type =
       m_symbol_file.GetOrCreateType(m_cvr.er.getUnderlyingType());
 
-  lldb::opaque_compiler_type_t enum_qt = m_derived_ct.GetOpaqueQualType();
-
-  CompilerType enumerator_type = clang.GetEnumerationIntegerType(enum_qt);
   uint64_t byte_size = underlying_type->GetByteSize();
   clang.AddEnumerationValueToEnumerationType(
-      m_derived_ct.GetOpaqueQualType(), enumerator_type, decl,
-      name.str().c_str(), enumerator.Value.getSExtValue(),
+      m_derived_ct, decl, name.str().c_str(), enumerator.Value.getSExtValue(),
       byte_size * 8);
   return Error::success();
 }
 
 void UdtRecordCompleter::complete() {
   ClangASTContext &clang = m_symbol_file.GetASTContext();
   clang.TransferBaseClasses(m_derived_ct.GetOpaqueQualType(),
                             std::move(m_bases));
 
   clang.AddMethodOverridesForCXXRecordType(m_derived_ct.GetOpaqueQualType());
   ClangASTContext::BuildIndirectFields(m_derived_ct);
   ClangASTContext::CompleteTagDeclarationDefinition(m_derived_ct);
 
   if (auto *record_decl = llvm::dyn_cast<clang::CXXRecordDecl>(&m_tag_decl)) {
     m_symbol_file.GetASTImporter().InsertRecordDecl(record_decl, m_layout);
   }
-}
+}
 

lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp

bool PDBASTParser::AddEnumValue(CompilerType enum_type,
   default:
     return false;
   }
   CompilerType underlying_type =
       m_ast.GetEnumerationIntegerType(enum_type.GetOpaqueQualType());
   uint32_t byte_size = m_ast.getASTContext()->getTypeSize(
       ClangUtil::GetQualType(underlying_type));
   auto enum_constant_decl = m_ast.AddEnumerationValueToEnumerationType(
-      enum_type.GetOpaqueQualType(), underlying_type, decl, name.c_str(),
-      raw_value, byte_size * 8);
+      enum_type, decl, name.c_str(), raw_value, byte_size * 8);
   if (!enum_constant_decl)
     return false;
 
   m_uid_to_decl[enum_value.getSymIndexId()] = enum_constant_decl;
 
   return true;
 }
 

lldb/trunk/source/Symbol/ClangASTContext.cpp

if (enutype) {
         return true;
       }
     }
   }
   return false;
 }
 
 clang::EnumConstantDecl *ClangASTContext::AddEnumerationValueToEnumerationType(
-    lldb::opaque_compiler_type_t type,
-    const CompilerType &enumerator_clang_type, const Declaration &decl,
-    const char *name, int64_t enum_value, uint32_t enum_value_bit_size) {
-  if (type && enumerator_clang_type.IsValid() && name && name[0]) {
-    clang::QualType enum_qual_type(GetCanonicalQualType(type));
+    const CompilerType &enum_type, const Declaration &decl, const char *name,
+    int64_t enum_value, uint32_t enum_value_bit_size) {
+
+  if (!enum_type || ConstString(name).IsEmpty())
+    return nullptr;
+
+  lldbassert(enum_type.GetTypeSystem() == static_cast<TypeSystem *>(this));
+
+  lldb::opaque_compiler_type_t enum_opaque_compiler_type =
+      enum_type.GetOpaqueQualType();
+
+  if (!enum_opaque_compiler_type)
+    return nullptr;
+
+  CompilerType underlying_type =
+      GetEnumerationIntegerType(enum_type.GetOpaqueQualType());
+
+  clang::QualType enum_qual_type(
+      GetCanonicalQualType(enum_opaque_compiler_type));
 
-    bool is_signed = false;
-    enumerator_clang_type.IsIntegerType(is_signed);
-    const clang::Type *clang_type = enum_qual_type.getTypePtr();
-    if (clang_type) {
-      const clang::EnumType *enutype =
-          llvm::dyn_cast<clang::EnumType>(clang_type);
+  underlying_type.IsIntegerType(is_signed);
 
-      if (enutype) {
-        llvm::APSInt enum_llvm_apsint(enum_value_bit_size, is_signed);
-        enum_llvm_apsint = enum_value;
-        clang::EnumConstantDecl *enumerator_decl =
-            clang::EnumConstantDecl::Create(
-                *getASTContext(), enutype->getDecl(), clang::SourceLocation(),
-                name ? &getASTContext()->Idents.get(name)
-                     : nullptr, // Identifier
-                ClangUtil::GetQualType(enumerator_clang_type),
-                nullptr, enum_llvm_apsint);
+  if (!clang_type)
+    return nullptr;
+
+  const clang::EnumType *enutype = llvm::dyn_cast<clang::EnumType>(clang_type);
+
+  if (!enutype)
+    return nullptr;
+
+  clang::EnumConstantDecl *enumerator_decl = clang::EnumConstantDecl::Create(
+      name ? &getASTContext()->Idents.get(name) : nullptr, // Identifier
+      clang::QualType(enutype, 0), nullptr, enum_llvm_apsint);
+
+  if (!enumerator_decl)
+    return nullptr;
 
-        if (enumerator_decl) {
-          enutype->getDecl()->addDecl(enumerator_decl);
 
 #ifdef LLDB_CONFIGURATION_DEBUG
-          VerifyDecl(enumerator_decl);
 #endif
 
-          return enumerator_decl;
-        }
-      }
-    }
-  }
-  return nullptr;
-}
 
 CompilerType
 ClangASTContext::GetEnumerationIntegerType(lldb::opaque_compiler_type_t type) {
   clang::QualType enum_qual_type(GetCanonicalQualType(type));
   const clang::Type *clang_type = enum_qual_type.getTypePtr();
   if (clang_type) {
     const clang::EnumType *enutype =
         llvm::dyn_cast<clang::EnumType>(clang_type);