[PATCH] D89286: [DebugInfo] Check for templated static data member when adding constant to record static fields

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1421-1425
+  else if (auto *TemplateDecl = Var->getInstantiatedFromStaticDataMember()) {
+    // Inline static data members might not have an initialization.
+    if (TemplateDecl->getInit())
+      Value = TemplateDecl->evaluateValue();
+  }
----------------
I'd suspect this might not be the most general solution - because it's retrieving the value from the original template static member. Two issues come to mind:
1) If the initialization is dependent or otherwise not knowable at compile-time, does this code at least not crash (I assume "evaluateValue" returns nullptr if the value can't be evaluated for whatever reason)
2) If the initialization is dependent, it might still be knowable, eg:

```
template<typename T>
struct static_decl_templ {
  static constexpr int static_constexpr_decl_templ_var = sizeof(T);
};
```

I'm guessing this patch as-is doesn't provide a value in this case, but it should be achievable, I would guess/hope/imagine - but don't really know exactly how to do that off-hand. (@zygoloid would know - but maybe a bit of looking around will find it if/before he has a chance to weigh in here)




================
Comment at: clang/test/CodeGenCXX/debug-info-static-member.cpp:140-144
+#if __cplusplus >= 201103L
+  static constexpr int static_constexpr_decl_templ_var = 8;
+#else
+  static const int static_constexpr_decl_templ_var = 8;
+#endif
----------------
I guess the non-11 version of this variable is intended as a stub to make the test across different language levels simpler?

I think it might be better to make that more explicit - rather than having a variable with "constexpr" in the name that isn't actually constexpr. I'd say only add the variable under that macro condition - but change the test to use multiple --check-prefixes, and the check for this variable to use a CPP11 prefix or something like that.

Hmm, I'm slightly confused now that I Think about it - it looks like the macro is about testing if the version is >= 11. But the change also adds a test for C++17? Is C++17 likely to be any different than C++11 for any of this? (or C++20 for that matter) if it's not any different, I'd probably leave the 17 case out & expect the 11 case is sufficient coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89286