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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

The only remaining issue with the LLDB expression part is that now that `SetFloatingInitializerForVariable` is based on the D81471 <https://reviews.llvm.org/D81471> version it no longer makes the member variable a `constexpr`. Int/Enum members with initialisers can just be const, but for any floating point types this ends ups creating an AST that we usually can't get from the Clang parser (as floats can't be initialised this way). Clang will still codegen that for us, but I think making the floating point VarDecls `constexpr` could save us from bad surprises in the future (e.g., someone adds an assert to Clang/Sema that const static data members are only initialised when they have integer/enum type). I think marking the variable as constexpr in the two places where you call `SetFloatingInitializerForVariable` seems like the right place for this (`SetFloatingInitializerForVariable` probably shouldn't set that flag on its own, that seems like unexpected behaviour from this function).

Otherwise I think `auto` is used quite frequently and doesn't really fit to LLVM's "auto only if it makes code more readable" policy. I think all the integer types for the type widths should just be their own type (`uint32_t` makes the code easier to understand than auto). And I don't think LLDB usually does `auto` in LLDB for types/decls as LLDB has its own type/decl abstraction (`CompilerType` and `CompilerDecl`), so that's also confusing.

Beside that this patch looks good to me. Maybe someone that knows more about PDB should also take a look about the PDB-specific parts of the patch.



================
Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7324-7331
+  // If the variable is an enum type, take the underlying integer type as
+  // the type of the integer literal.
+  if (const EnumType *enum_type = llvm::dyn_cast<EnumType>(qt.getTypePtr())) {
+    const EnumDecl *enum_decl = enum_type->getDecl();
+    qt = enum_decl->getIntegerType();
+  }
+  var->setInit(IntegerLiteral::Create(ast, init_value, qt.getUnqualifiedType(),
----------------
aleksandr.urakov wrote:
> I'm not sure, can we initialize a member this way if it has a scoped enum type?
(That code is from D81471 so I think I should answer that)

Well, it works and but it relies on CodeGen/Sema not double-checking that we get the enum type (and not the underlying int type). I can inject a CXXStaticCastExpr too and will update D81471. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82160