[Lldb-commits] [PATCH] D82160: [lldb][PDB] Constexpr static member values as AST literals

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

You might also be interested in D81471 <https://reviews.llvm.org/D81471> which is a very similar patch but for DWARF and only focused on static const integer/enum data members.

One thing I'm curious about is how this is solving (or if it's even supposed to solve) the issue that the expression parser is by default taking the address of lvalues (which will bring us back to the issue that LLDB will try to resolve the symbol)? From what I understand, this patch allows me to to things like `expr Class::Member + 1` but not `expr Class::Member` (because the expression evaluator will rewrite that lvalue to `int *$__lldb_expr_result_ptr = &Class::Member`).



================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:155
+          switch (ClangUtil::GetQualType(member_ct)
+                      ->castAs<clang::BuiltinType>()
+                      ->getKind()) {
----------------
What about just doing the switch directly on `member_ct.GetBasicTypeEnumeration()` and check for `eBasicTypeFloat` and so on. That's less code to do the same thing without all the casting.


================
Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7321
+  var_decl->setInit(clang::IntegerLiteral::Create(
+      var_decl->getASTContext(), value.extOrTrunc(num_bits),
+      var_decl->getType(), clang::SourceLocation()));
----------------
Passing a too large APSInt into this function will just silently truncate it. I think this should refuse to add the initializer in this case (and we probably should at least log that error somewhere).


================
Comment at: lldb/test/Shell/SymbolFile/PDB/Inputs/AstRestoreTest.cpp:27
+  static constexpr float ClassStaticConstexprFloat = 9.f;
+  static constexpr double ClassStaticConstexprDouble = 10.0;
 
----------------
You might want to also add some static const (not `constexpr`) integer and enum members to this test.

Also I'm curious what happens if you put a `long double` as a member? From what I can see the expected behavior is that PDB will not emit that as a constant (as the current code asserts if it encounters `long double` anywhere).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82160/new/

https://reviews.llvm.org/D82160