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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

@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.


Repository:
  rL LLVM

https://reviews.llvm.org/D54003



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Shafik Yaghmour via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346428: Refactor 
ClangASTContext::AddEnumerationValueToEnumerationType() to remove… (authored by 
shafik, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54003?vs=173083=173197#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54003

Files:
  lldb/trunk/include/lldb/Symbol/ClangASTContext.h
  
lldb/trunk/packages/Python/lldbsuite/test/expression_command/radar_43822994/Makefile
  
lldb/trunk/packages/Python/lldbsuite/test/expression_command/radar_43822994/TestScopedEnumType.py
  
lldb/trunk/packages/Python/lldbsuite/test/expression_command/radar_43822994/main.cpp
  lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  lldb/trunk/source/Symbol/ClangASTContext.cpp

Index: lldb/trunk/source/Symbol/ClangASTContext.cpp
===
--- lldb/trunk/source/Symbol/ClangASTContext.cpp
+++ lldb/trunk/source/Symbol/ClangASTContext.cpp
@@ -8876,43 +8876,55 @@
 }
 
 clang::EnumConstantDecl *ClangASTContext::AddEnumerationValueToEnumerationType(
-lldb::opaque_compiler_type_t type,
-const CompilerType _clang_type, const Declaration ,
-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));
-
-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_type);
+const CompilerType _type, const Declaration , 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(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;
+  underlying_type.IsIntegerType(is_signed);
+  const clang::Type *clang_type = enum_qual_type.getTypePtr();
 
-  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 ? ()->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_type);
+
+  if (!enutype)
+return nullptr;
+
+  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 ? ()->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);
+  enutype->getDecl()->addDecl(enumerator_decl);
 
 #ifdef LLDB_CONFIGURATION_DEBUG
-  VerifyDecl(enumerator_decl);
+  VerifyDecl(enumerator_decl);
 #endif
 
-  return enumerator_decl;
-}
-  }
-}
-  }
-  return nullptr;
+  return enumerator_decl;
 }
 
 CompilerType
Index: lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -1122,8 +1122,7 @@
   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;
 
Index: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ 

[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 accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.

Looks good to me.


https://reviews.llvm.org/D54003



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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 _qual_type, const Declaration ,
-  const char *name, int64_t enum_value, uint32_t enum_value_bit_size);
+  const CompilerType enum_type, const Declaration , 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



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik updated this revision to Diff 173083.
shafik marked 8 inline comments as done.
shafik added a comment.

Made changes based on comments.


https://reviews.llvm.org/D54003

Files:
  include/lldb/Symbol/ClangASTContext.h
  packages/Python/lldbsuite/test/expression_command/radar_43822994/Makefile
  
packages/Python/lldbsuite/test/expression_command/radar_43822994/TestScopedEnumType.py
  packages/Python/lldbsuite/test/expression_command/radar_43822994/main.cpp
  source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  source/Symbol/ClangASTContext.cpp

Index: source/Symbol/ClangASTContext.cpp
===
--- source/Symbol/ClangASTContext.cpp
+++ source/Symbol/ClangASTContext.cpp
@@ -8876,43 +8876,55 @@
 }
 
 clang::EnumConstantDecl *ClangASTContext::AddEnumerationValueToEnumerationType(
-lldb::opaque_compiler_type_t type,
-const CompilerType _clang_type, const Declaration ,
-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));
-
-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_type);
-
-  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 ? ()->Idents.get(name)
- : nullptr, // Identifier
-ClangUtil::GetQualType(enumerator_clang_type),
-nullptr, enum_llvm_apsint);
-
-if (enumerator_decl) {
-  enutype->getDecl()->addDecl(enumerator_decl);
+const CompilerType _type, const Declaration , 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(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;
+  underlying_type.IsIntegerType(is_signed);
+  const clang::Type *clang_type = enum_qual_type.getTypePtr();
+
+  if (!clang_type)
+return nullptr;
+
+  const clang::EnumType *enutype = llvm::dyn_cast(clang_type);
+
+  if (!enutype)
+return nullptr;
+
+  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 ? ()->Idents.get(name) : nullptr, // Identifier
+  clang::QualType(enutype, 0), nullptr, enum_llvm_apsint);
+
+  if (!enumerator_decl)
+return nullptr;
+
+  enutype->getDecl()->addDecl(enumerator_decl);
 
 #ifdef LLDB_CONFIGURATION_DEBUG
-  VerifyDecl(enumerator_decl);
+  VerifyDecl(enumerator_decl);
 #endif
 
-  return enumerator_decl;
-}
-  }
-}
-  }
-  return nullptr;
+  return enumerator_decl;
 }
 
 CompilerType
Index: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -1122,8 +1122,7 @@
   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;
 
Index: source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
===
--- source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
+++ source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
@@ -159,13 +159,9 @@
   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();
 

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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: include/lldb/Symbol/ClangASTContext.h:909
   clang::EnumConstantDecl *AddEnumerationValueToEnumerationType(
-  lldb::opaque_compiler_type_t type,
-  const CompilerType _qual_type, const Declaration ,
-  const char *name, int64_t enum_value, uint32_t enum_value_bit_size);
+  const CompilerType enum_type, const Declaration , const char *name,
+  int64_t enum_value, uint32_t enum_value_bit_size);

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!


https://reviews.llvm.org/D54003



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

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


https://reviews.llvm.org/D54003



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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 _qual_type, const Declaration ,
-  const char *name, int64_t enum_value, uint32_t enum_value_bit_size);
+  const CompilerType enum_type, const Declaration , 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



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

@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.


https://reviews.llvm.org/D54003



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Zachary Turner via lldb-commits]

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?
On Wed, Nov 7, 2018 at 2:20 PM Shafik Yaghmour via Phabricator <
revi...@reviews.llvm.org> wrote:

> shafik added a comment.
>
> @zturner I don't see `AddEnumerationValueToEnumerationType` being called
> in `SymbolFile/NativePDB.cpp`
> https://github.com/llvm-mirror/lldb/search?q=AddEnumerationValueToEnumerationType_q=AddEnumerationValueToEnumerationType
>
>
> https://reviews.llvm.org/D54003
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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 a subscriber: shafik.
zturner added a comment.

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?


https://reviews.llvm.org/D54003



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

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


https://reviews.llvm.org/D54003



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

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.




Comment at: include/lldb/Symbol/ClangASTContext.h:909
   clang::EnumConstantDecl *AddEnumerationValueToEnumerationType(
-  lldb::opaque_compiler_type_t type,
-  const CompilerType _qual_type, const Declaration ,
-  const char *name, int64_t enum_value, uint32_t enum_value_bit_size);
+  const CompilerType enum_type, const Declaration , const char *name,
+  int64_t enum_value, uint32_t enum_value_bit_size);

teemperor wrote:
> Can we pass `enum_type` as const ref?
CompilerType instances are two pointers. Pretty cheap to copy.



Comment at: source/Symbol/ClangASTContext.cpp:8851
 clang::EnumConstantDecl *ClangASTContext::AddEnumerationValueToEnumerationType(
-lldb::opaque_compiler_type_t type,
-const CompilerType _clang_type, const Declaration ,
-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));
-
-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_type);
-
-  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 ? ()->Idents.get(name)
- : nullptr, // Identifier
-ClangUtil::GetQualType(enumerator_clang_type),
-nullptr, enum_llvm_apsint);
-
-if (enumerator_decl) {
-  enutype->getDecl()->addDecl(enumerator_decl);
+const CompilerType enum_type, const Declaration , const char *name,
+int64_t enum_value, uint32_t enum_value_bit_size) {

teemperor wrote:
> (Same as above) Can we have `enum_type` as a const ref?
CompilerType instances are two pointers. Pretty cheap to copy.



Comment at: source/Symbol/ClangASTContext.cpp:8853
+int64_t enum_value, uint32_t enum_value_bit_size) {
+
+  if (!enum_type || ConstString(name).IsEmpty())

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


https://reviews.llvm.org/D54003



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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 a comment.

This function is called in `SymbolFile/NativePDB` as well.


https://reviews.llvm.org/D54003



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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




Comment at: include/lldb/Symbol/ClangASTContext.h:909
   clang::EnumConstantDecl *AddEnumerationValueToEnumerationType(
-  lldb::opaque_compiler_type_t type,
-  const CompilerType _qual_type, const Declaration ,
-  const char *name, int64_t enum_value, uint32_t enum_value_bit_size);
+  const CompilerType enum_type, const Declaration , const char *name,
+  int64_t enum_value, uint32_t enum_value_bit_size);

Can we pass `enum_type` as const ref?



Comment at: source/Symbol/ClangASTContext.cpp:8851
 clang::EnumConstantDecl *ClangASTContext::AddEnumerationValueToEnumerationType(
-lldb::opaque_compiler_type_t type,
-const CompilerType _clang_type, const Declaration ,
-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));
-
-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_type);
-
-  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 ? ()->Idents.get(name)
- : nullptr, // Identifier
-ClangUtil::GetQualType(enumerator_clang_type),
-nullptr, enum_llvm_apsint);
-
-if (enumerator_decl) {
-  enutype->getDecl()->addDecl(enumerator_decl);
+const CompilerType enum_type, const Declaration , const char *name,
+int64_t enum_value, uint32_t enum_value_bit_size) {

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


https://reviews.llvm.org/D54003



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Davide Italiano via Phabricator via lldb-commits]

davide added inline comments.



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())

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


https://reviews.llvm.org/D54003



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik created this revision.
shafik added reviewers: jingham, davide, teemperor.
Herald added a subscriber: JDevlieghere.

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


https://reviews.llvm.org/D54003

Files:
  include/lldb/Symbol/ClangASTContext.h
  packages/Python/lldbsuite/test/expression_command/radar_43822994/Makefile
  
packages/Python/lldbsuite/test/expression_command/radar_43822994/TestScopedEnumType.py
  packages/Python/lldbsuite/test/expression_command/radar_43822994/main.cpp
  source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  source/Symbol/ClangASTContext.cpp

Index: source/Symbol/ClangASTContext.cpp
===
--- source/Symbol/ClangASTContext.cpp
+++ source/Symbol/ClangASTContext.cpp
@@ -8848,43 +8848,53 @@
 }
 
 clang::EnumConstantDecl *ClangASTContext::AddEnumerationValueToEnumerationType(
-lldb::opaque_compiler_type_t type,
-const CompilerType _clang_type, const Declaration ,
-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));
-
-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_type);
-
-  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 ? ()->Idents.get(name)
- : nullptr, // Identifier
-ClangUtil::GetQualType(enumerator_clang_type),
-nullptr, enum_llvm_apsint);
-
-if (enumerator_decl) {
-  enutype->getDecl()->addDecl(enumerator_decl);
+const CompilerType enum_type, const Declaration , const char *name,
+int64_t enum_value, uint32_t enum_value_bit_size) {
+
+  if (!enum_type || ConstString(name).IsEmpty())
+return nullptr;
+
+  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;
+  underlying_type.IsIntegerType(is_signed);
+  const clang::Type *clang_type = enum_qual_type.getTypePtr();
+
+  if (!clang_type)
+return nullptr;
+
+  const clang::EnumType *enutype = llvm::dyn_cast(clang_type);
+
+  if (!enutype)
+return nullptr;
+
+  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 ? ()->Idents.get(name) : nullptr, // Identifier
+  clang::QualType(enutype, 0), nullptr, enum_llvm_apsint);
+
+  if (!enumerator_decl)
+return nullptr;
+
+  enutype->getDecl()->addDecl(enumerator_decl);
 
 #ifdef LLDB_CONFIGURATION_DEBUG
-  VerifyDecl(enumerator_decl);
+  VerifyDecl(enumerator_decl);
 #endif
 
-  return enumerator_decl;
-}
-  }
-}
-  }
-  return nullptr;
+  return enumerator_decl;
 }
 
 CompilerType
Index: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -1086,8 +1086,7 @@
   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;
 
Index: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2501,9 +2501,7 @@
 
 if (name && name[0] && got_value) {
   m_ast.AddEnumerationValueToEnumerationType(
-