[lldb][NFC] Use the DWARFExpression::Stack typedef #167018
Conversation
This commit updates the places where we were passing around the dwarf expression stack to use the typedef value instead of redeclaring the underlying vector type. It looked like a nice cleanup to use the typedef instead.
|
@llvm/pr-subscribers-lldb Author: David Peixotto (dmpots) ChangesThis commit updates the places where we were passing around the dwarf expression stack to use the typedef value instead of redeclaring the underlying vector type. It looked like a nice cleanup to use the typedef instead. Full diff: https://github.com/llvm/llvm-project/pull/167018.diff 8 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
index 94fc2e83e899d..ea1ad71f0b3dc 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -9,6 +9,7 @@
#include "DWARFUnit.h"
#include "lldb/Core/Module.h"
+#include "lldb/Expression/DWARFExpression.h"
#include "lldb/Symbol/ObjectFile.h"
#include "lldb/Utility/LLDBAssert.h"
#include "lldb/Utility/StreamString.h"
@@ -738,7 +739,7 @@ bool DWARFUnit::ParseVendorDWARFOpcode(uint8_t op, const DataExtractor &opcodes,
lldb::offset_t &offset,
RegisterContext *reg_ctx,
lldb::RegisterKind reg_kind,
- std::vector<Value> &stack) const {
+ DWARFExpression::Stack &stack) const {
return GetSymbolFileDWARF().ParseVendorDWARFOpcode(op, opcodes, offset,
reg_ctx, reg_kind, stack);
}
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
index 91a693860c55a..80a1ccf794f8b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
@@ -164,11 +164,11 @@ class DWARFUnit : public DWARFExpression::Delegate, public UserID {
const lldb::offset_t data_offset,
const uint8_t op) const override;
- virtual bool ParseVendorDWARFOpcode(uint8_t op, const DataExtractor &opcodes,
- lldb::offset_t &offset,
- RegisterContext *reg_ctx,
- lldb::RegisterKind reg_kind,
- std::vector<Value> &stack) const override;
+ virtual bool
+ ParseVendorDWARFOpcode(uint8_t op, const DataExtractor &opcodes,
+ lldb::offset_t &offset, RegisterContext *reg_ctx,
+ lldb::RegisterKind reg_kind,
+ DWARFExpression::Stack &stack) const override;
bool ParseDWARFLocationList(const DataExtractor &data,
DWARFExpressionList &loc_list) const;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
index a60527b8eda33..018a617bb85bf 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -22,6 +22,7 @@
#include "lldb/Core/UniqueCStringMap.h"
#include "lldb/Core/dwarf.h"
+#include "lldb/Expression/DWARFExpression.h"
#include "lldb/Expression/DWARFExpressionList.h"
#include "lldb/Symbol/DebugMacros.h"
#include "lldb/Symbol/SymbolContext.h"
@@ -334,7 +335,7 @@ class SymbolFileDWARF : public SymbolFileCommon {
lldb::offset_t &offset,
RegisterContext *reg_ctx,
lldb::RegisterKind reg_kind,
- std::vector<Value> &stack) const {
+ DWARFExpression::Stack &stack) const {
return false;
}
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
index 60d87c3fc2559..82b7999be1130 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
@@ -99,7 +99,7 @@ uint64_t SymbolFileDWARFDwo::GetDebugInfoSize(bool load_all_debug_info) {
bool SymbolFileDWARFDwo::ParseVendorDWARFOpcode(
uint8_t op, const DataExtractor &opcodes, lldb::offset_t &offset,
RegisterContext *reg_ctx, lldb::RegisterKind reg_kind,
- std::vector<Value> &stack) const {
+ DWARFExpression::Stack &stack) const {
return GetBaseSymbolFile().ParseVendorDWARFOpcode(op, opcodes, offset,
reg_ctx, reg_kind, stack);
}
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
index d906e09fe81ab..1c4819ff9dafa 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
@@ -54,7 +54,7 @@ class SymbolFileDWARFDwo : public SymbolFileDWARF {
bool ParseVendorDWARFOpcode(uint8_t op, const DataExtractor &opcodes,
lldb::offset_t &offset, RegisterContext *reg_ctx,
lldb::RegisterKind reg_kind,
- std::vector<Value> &stack) const override;
+ DWARFExpression::Stack &stack) const override;
void FindGlobalVariables(ConstString name,
const CompilerDeclContext &parent_decl_ctx,
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileWasm.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileWasm.cpp
index e25a89cfe26b2..f53b9f767f495 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileWasm.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileWasm.cpp
@@ -46,12 +46,10 @@ SymbolFileWasm::GetVendorDWARFOpcodeSize(const DataExtractor &data,
return offset - data_offset;
}
-bool SymbolFileWasm::ParseVendorDWARFOpcode(uint8_t op,
- const DataExtractor &opcodes,
- lldb::offset_t &offset,
- RegisterContext *reg_ctx,
- lldb::RegisterKind reg_kind,
- std::vector<Value> &stack) const {
+bool SymbolFileWasm::ParseVendorDWARFOpcode(
+ uint8_t op, const DataExtractor &opcodes, lldb::offset_t &offset,
+ RegisterContext *reg_ctx, lldb::RegisterKind reg_kind,
+ DWARFExpression::Stack &stack) const {
if (op != llvm::dwarf::DW_OP_WASM_location)
return false;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileWasm.h b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileWasm.h
index 0e0b742f53f18..6ab2e0871a1a5 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileWasm.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileWasm.h
@@ -10,6 +10,7 @@
#define LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_SYMBOLFILEWASM_H
#include "SymbolFileDWARF.h"
+#include "lldb/Expression/DWARFExpression.h"
namespace lldb_private::plugin {
namespace dwarf {
@@ -26,7 +27,7 @@ class SymbolFileWasm : public SymbolFileDWARF {
bool ParseVendorDWARFOpcode(uint8_t op, const DataExtractor &opcodes,
lldb::offset_t &offset, RegisterContext *reg_ctx,
lldb::RegisterKind reg_kind,
- std::vector<Value> &stack) const override;
+ DWARFExpression::Stack &stack) const override;
};
} // namespace dwarf
} // namespace lldb_private::plugin
diff --git a/lldb/unittests/Expression/DWARFExpressionTest.cpp b/lldb/unittests/Expression/DWARFExpressionTest.cpp
index 9d11060becfae..d09224a70395e 100644
--- a/lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ b/lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -743,12 +743,12 @@ class CustomSymbolFileDWARF : public SymbolFileDWARF {
return offset - data_offset;
}
- virtual bool ParseVendorDWARFOpcode(
- uint8_t op, const lldb_private::DataExtractor &opcodes,
- lldb::offset_t &offset,
+ virtual bool
+ ParseVendorDWARFOpcode(uint8_t op, const lldb_private::DataExtractor &opcodes,
+ lldb::offset_t &offset,
- RegisterContext *reg_ctx, lldb::RegisterKind reg_kind,
- std::vector<lldb_private::Value> &stack) const override {
+ RegisterContext *reg_ctx, lldb::RegisterKind reg_kind,
+ DWARFExpression::Stack &stack) const override {
if (op != DW_OP_WASM_location) {
return false;
}
|
dmpots
changed the title
dmpots
changed the title
|
We could also just remove the typedef, as it doesn't tell the reader anything about the type; DEARFExpression::Stack is trying to say what the variable is supposed to be (which is better captured by the variable name), not how the concrete type behaves. |
|
Further evidence of this is that now we have "stack" repeated twice |
I think the typedef is still useful because it ties it back to the dwarf expression evaluation. The context is useful because we know that the stack being passed is the dwarf expression stack. It's not the fact that it is a stack that is useful, but that it is the dwarf expression stack. |
|
Right, that's exactly what I'm trying to say: we can keep the original type and rename the variable instead. The concrete type is still useful for readers to know how it can be used. We don't treat the concrete type as a stack (e.g. we manipulate it in the middle), so having "stack" in the type is very misleading. |
|
I see where you're coming from, but to me this seems like a reasonable use of a typedef. I see them as a way to (1) create a shorter or more meaningful names for a type and/or (2) to encapsulate/abstract implementation details that may change. Both apply here. The fact that the DWARF stack is implemented through a vector is mostly irrelevant, or at least secondary to it conceptually being a stack. Admittedly, the typedef isn't much shorter (if that was the goal we could use The repetition between the type and the name doesn't seem like a counter-argument. It's prevalent in the code base, including in combination with typedefs like |
|
Personally I have rarely actually found a typedef like this useful and almost always need to go back and forth to its definition. I've seen LLVM PRs that do exactly what @felipepiovezan suggested. I'd personally only use it to make primitve types more strongly typed (like the |
Personally I don't think character length should ever be an argument to change a type. I'd agree if we were dropping a namespace while preserving the actual type.
I'm not sure I can agree with this, to me it is the opposite of meaningful. std::vector is the definition of meaningfulness here, it can't get more meaningful than that, as this is what the type is.
If we were creating a new type, with stack-like operations, then we are abstracting. But here we actually want the type to be a vector, since it is useful to implement what this variable is representing, which is a dwarf expression stack. By keeping the type written as is, we are communicating this to future programmers: we have a vector because it is useful for the implementation. By having the typedef, we are hiding this from future programmers, who will likely be confused when they see an operator [], or some other operation that does not belong on a real stack type. |
|
There's a whole Wikipedia article dedicated to typedefs. It lists some of the motivations I mentioned and includes a section with concerns that matches Felipe's and Michael's objections. Clearly it's a somewhat subjective and controversial topic. Usually we settle those arguments by picking one side and adding it to the Coding Standards. I honestly don't care which one we settle on as long as we're consistent. |
|
@JDevlieghere @felipepiovezan What should be the next steps here? Do we need to spin up a discussion with the community? |
|
@felipepiovezan Do you feel strongly about this? Do you want @dmpots to remove the typedef? |
I wouldn't ask @dmpots to remove the typedef, as that is more work. I would just close this PR and not merge it |
|
I believe any discussion with the whole LLVM community would invariably converge to lack of consensus (aka diverge 😓 ) and decide on a case-by-case basis. Worth a try if you are so motivated. If we go on a case-by-case basis in the meantime, I can still be persuaded, but AFAICT we have not yet provided any arguments for why this should be changed or any counter to the points I raised above. |
|
To summarize my core points:
|
|
@felipepiovezan I stumbled upon this change when I was experimenting with changing the type of the dwarf expression stack. That's when I noticed we have uses that were not covered by the typedef. I do not feel super strongly about this change, but I do think it is an improvement over the existing code. I think the typedef is useful because:
I think that shorter names are actually useful sometimes, but that doesn't apply here so we can ignore it.
The typedef gives it a meaningful name. Just like variables we like to have meaningful names for our types to make them easier to reason about.
The fact that the concrete implementation is a vector is mostly irrelevant. If we do need to know the concrete type it is easily found from the typedef. |
5 participants
This commit updates the places where we were passing around the dwarf expression stack to use the typedef value instead of redeclaring the underlying vector type.
It looked like a nice cleanup to use the typedef instead.