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

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
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);
----------------
shafik wrote:
> jingham wrote:
> > 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".
> That was a good catch, thank you!
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.


https://reviews.llvm.org/D54003