[Lldb-commits] [PATCH] D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter

[Jim Ingham via Phabricator via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: include/lldb/Symbol/ClangASTContext.h:909
   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);
----------------
clayborg wrote:
> teemperor wrote:
> > Can we pass `enum_type` as const ref?
> CompilerType instances are two pointers. Pretty cheap to copy.
We're not entirely consistent, but a quick glance through headers show us almost always choosing to pass "const CompilerType &" over "const CompilerType".


================
Comment at: packages/Python/lldbsuite/test/expression_command/radar_43822994/TestScopedEnumType.py:42
+        ## integral is not implicitly convertible to a scoped enum
+        value = frame.EvaluateExpression("1 == Foo::FooBar")
+        self.assertTrue(value.IsValid())
----------------
davide wrote:
> This clearly looks like can be an inline test (and probably would make the thing more readable, IMHO).
The form of test to use is up to the test writer, seems to me.  This test is not at all hard to read.


https://reviews.llvm.org/D54003