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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
