[PATCH] D70486: Make DebugVariable class available in DebugInfoMetadata

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

How about adding a quick unit test in the metadata unit test file?



================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:3266
+/// same variable instance, but not that one DebugVariable points to multiple
+/// variable instances.
+class DebugVariable {
----------------
thanks!


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:3284
+
+  DebugVariable(const DILocalVariable *Var, OptFragmentInfo &FragmentInfo,
+                const DILocation *InlinedAt)
----------------
Orlando wrote:
> Could this be `const OptFragmentInfo &`?
FragmentInfo small and meant to be used as a value type. I would prefer

`Optional<FragmentInfo>` (literally, without the `using` to hide the Optional). It's easier to read.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:3293
+  const DILocalVariable *getVar() const { return Variable; }
+  const OptFragmentInfo &getFragment() const { return Fragment; }
+  const DILocation *getInlinedAt() const { return InlinedAt; }
----------------
Again, let's just return this by value.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:3296
+
+  const FragmentInfo getFragmentDefault() const {
+    return Fragment.getValueOr(DefaultFragment);
----------------
This warrants a comment explaining what this function does.

Is this actually more readable than `Fragment.getValueOr(DefaultFragment)`?


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:3300
+
+  static bool isFragmentDefault(const FragmentInfo &F) {
+    return F == DefaultFragment;
----------------
isDefaultFragment?


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:3320
+
+  // Empty key: no key should be generated that has no DILocalVariable.
+  static inline DV getEmptyKey() {
----------------
///


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:3322
+  static inline DV getEmptyKey() {
+    return DV(nullptr, OptFragmentInfo(), nullptr);
+  }
----------------
`return DV(nullptr, {}, nullptr)`


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:3325
+
+  // Difference in tombstone is that the Optional is meaningful
+  static inline DV getTombstoneKey() {
----------------
/// ... .


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:3327
+  static inline DV getTombstoneKey() {
+    return DV(nullptr, OptFragmentInfo({0, 0}), nullptr);
+  }
----------------
 DV(nullptr, {0, 0}, nullptr);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70486