clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
See inlined comments.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:179
@@ -178,3 +178,3 @@
bool prev_die_had_children = false;
- const uint8_t *fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize (GetAddressByteSize(),
m_is_dwarf64);
+ auto fixed_form_sizes = DWARFFormValue::GetFixedFormSizesForAddressSize
(GetAddressByteSize(), m_is_dwarf64);
while (offset < next_cu_offset &&
----------------
Please don't use auto for simple types. For template types yes (for things like
"std::vector<int>::iterator", but not for simple types.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:665
@@ -664,3 +664,3 @@
- const uint8_t *fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize (GetAddressByteSize(),
m_is_dwarf64);
+ auto fixed_form_sizes = DWARFFormValue::GetFixedFormSizesForAddressSize
(GetAddressByteSize(), m_is_dwarf64);
----------------
Please don't use auto for simple types. For template types yes (for things like
"std::vector<int>::iterator", but not for simple types.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:161
@@ -160,3 +160,3 @@
- const uint8_t fixed_skip_size = fixed_form_sizes [form];
+ const uint8_t fixed_skip_size = form < fixed_form_sizes->size() ?
fixed_form_sizes->at(form) : 0;
if (fixed_skip_size)
----------------
This should be a method on DWARFFormValue::FixedFormSizes.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1286
@@ -1285,3 +1285,3 @@
{
- const uint8_t fixed_skip_size = fixed_form_sizes [form];
+ const uint8_t fixed_skip_size = form <
fixed_form_sizes->size() ? fixed_form_sizes->at(form) : 0;
if (fixed_skip_size)
----------------
This should be a method on DWARFFormValue::FixedFormSizes.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugPubnames.cpp:91
@@ -90,3 +90,3 @@
- const uint8_t *fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize (cu->GetAddressByteSize(),
cu->IsDWARF64());
+ auto fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize (cu->GetAddressByteSize(),
cu->IsDWARF64());
----------------
"DWARFFormValue::FixedFormSizes *" instead of "auto".
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp:23-24
@@ -22,4 +22,4 @@
-static uint8_t g_form_sizes_addr4[] =
-{
+static DWARFFormValue::FixedFormSizes
+g_form_sizes_addr4 {
0, // 0x00 unused
----------------
You could probably leave these as "static uint8_t g_form_sizes_addr4[]" and
then have the new DWARFFormValue::FixedFormSizes struct/class that we make get
intialized with these const arrays and do the bounds checking. This would avoid
static constructors and also provide bounds checking.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp:60-61
@@ -60,5 +59,4 @@
-static uint8_t
-g_form_sizes_addr8[] =
-{
+static DWARFFormValue::FixedFormSizes
+g_form_sizes_addr8 {
0, // 0x00 unused
----------------
You could probably leave these as "static uint8_t g_form_sizes_addr8[]" and
then have the new DWARFFormValue::FixedFormSizes struct/class that we make get
intialized with these const arrays and do the bounds checking. This would avoid
static constructors and also provide bounds checking.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp:99-100
@@ -100,5 +98,4 @@
// DW_FORM_strp and DW_FORM_sec_offset are 8 instead of 4
-static uint8_t
-g_form_sizes_addr8_dwarf64[] =
-{
+static DWARFFormValue::FixedFormSizes
+g_form_sizes_addr8_dwarf64 {
0, // 0x00 unused
----------------
Ditto above comment.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp:136-151
@@ -138,18 +135,18 @@
-const uint8_t *
+DWARFFormValue::FixedFormSizes*
DWARFFormValue::GetFixedFormSizesForAddressSize (uint8_t addr_size, bool
is_dwarf64)
{
if (!is_dwarf64) {
switch (addr_size)
{
- case 4: return g_form_sizes_addr4;
- case 8: return g_form_sizes_addr8;
+ case 4: return &g_form_sizes_addr4;
+ case 8: return &g_form_sizes_addr8;
}
} else {
if (addr_size == 8)
- return g_form_sizes_addr8_dwarf64;
+ return &g_form_sizes_addr8_dwarf64;
// is_dwarf64 && addr_size == 4 : no provider does this.
}
- return NULL;
+ return nullptr;
}
----------------
Change this function to use the original static arrays and construct a
DWARFFormValue::FixedFormSizes with a pointer and array size to provide bounds
checking. No need to use a std::vector for const data.
================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormValue.h:39
@@ -38,1 +38,3 @@
+
+ typedef const std::vector<uint8_t> FixedFormSizes;
----------------
labath wrote:
> I don't think hiding const in the type name is a good idea. It makes it quite
> hard to tell you are declaring constant arrays below.
Make this a struct and add a method to access the form size by form so we
don't have the following code in multiple places:
```
const uint8_t fixed_skip_size = form < fixed_form_sizes->size() ?
fixed_form_sizes->at(form) : 0;
```
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:4056
@@ -4055,3 +4055,3 @@
// Retrieve the value as a data expression.
- const uint8_t *fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize
(attributes.CompileUnitAtIndex(i)->GetAddressByteSize(),
attributes.CompileUnitAtIndex(i)->IsDWARF64());
+ auto fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize
(attributes.CompileUnitAtIndex(i)->GetAddressByteSize(),
attributes.CompileUnitAtIndex(i)->IsDWARF64());
uint32_t data_offset =
attributes.DIEOffsetAtIndex(i);
----------------
No auto
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:4058
@@ -4057,3 +4057,3 @@
uint32_t data_offset =
attributes.DIEOffsetAtIndex(i);
- uint32_t data_length =
fixed_form_sizes[form_value.Form()];
+ uint32_t data_length = form_value.Form() <
fixed_form_sizes->size() ? fixed_form_sizes->at(form_value.Form()) : 0;
if (data_length == 0)
----------------
use DWARFFormValue::FixedFormSizes form accessor function.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:4080
@@ -4079,3 +4079,3 @@
{
- const uint8_t *fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize
(attributes.CompileUnitAtIndex(i)->GetAddressByteSize(),
attributes.CompileUnitAtIndex(i)->IsDWARF64());
+ auto fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize
(attributes.CompileUnitAtIndex(i)->GetAddressByteSize(),
attributes.CompileUnitAtIndex(i)->IsDWARF64());
uint32_t data_offset =
attributes.DIEOffsetAtIndex(i);
----------------
no auto
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:4082
@@ -4081,3 +4081,3 @@
uint32_t data_offset =
attributes.DIEOffsetAtIndex(i);
- uint32_t data_length =
fixed_form_sizes[form_value.Form()];
+ uint32_t data_length = form_value.Form() <
fixed_form_sizes->size() ? fixed_form_sizes->at(form_value.Form()) : 0;
location.CopyOpcodeData(module,
debug_info_data, data_offset, data_length);
----------------
use DWARFFormValue::FixedFormSizes form accessor function.
================
Comment at: source/Symbol/ClangASTContext.cpp:8944
@@ -8943,3 +8943,3 @@
{
- const uint8_t *fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize
(dwarf_cu->GetAddressByteSize(), dwarf_cu->IsDWARF64());
+ auto fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize
(dwarf_cu->GetAddressByteSize(), dwarf_cu->IsDWARF64());
----------------
no auto
================
Comment at: source/Symbol/ClangASTContext.cpp:9464
@@ -9463,3 +9463,3 @@
const DWARFDebugInfoEntry *die;
- const uint8_t *fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize
(dwarf_cu->GetAddressByteSize(), dwarf_cu->IsDWARF64());
+ auto fixed_form_sizes = DWARFFormValue::GetFixedFormSizesForAddressSize
(dwarf_cu->GetAddressByteSize(), dwarf_cu->IsDWARF64());
----------------
no auto
================
Comment at: source/Symbol/ClangASTContext.cpp:9823
@@ -9822,3 +9822,3 @@
const DWARFDebugInfoEntry *die;
- const uint8_t *fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize
(dwarf_cu->GetAddressByteSize(), dwarf_cu->IsDWARF64());
+ auto fixed_form_sizes = DWARFFormValue::GetFixedFormSizesForAddressSize
(dwarf_cu->GetAddressByteSize(), dwarf_cu->IsDWARF64());
uint32_t member_idx = 0;
----------------
no auto
================
Comment at: source/Symbol/ClangASTContext.cpp:10405
@@ -10404,3 +10404,3 @@
- const uint8_t *fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize
(dwarf_cu->GetAddressByteSize(), dwarf_cu->IsDWARF64());
+ auto fixed_form_sizes = DWARFFormValue::GetFixedFormSizesForAddressSize
(dwarf_cu->GetAddressByteSize(), dwarf_cu->IsDWARF64());
----------------
no auto
================
Comment at: source/Symbol/ClangASTContext.cpp:10583
@@ -10582,3 +10582,3 @@
const DWARFDebugInfoEntry *die;
- const uint8_t *fixed_form_sizes =
DWARFFormValue::GetFixedFormSizesForAddressSize
(dwarf_cu->GetAddressByteSize(), dwarf_cu->IsDWARF64());
+ auto fixed_form_sizes = DWARFFormValue::GetFixedFormSizesForAddressSize
(dwarf_cu->GetAddressByteSize(), dwarf_cu->IsDWARF64());
for (die = parent_die->GetFirstChild(); die != NULL; die =
die->GetSibling())
----------------
no auto
http://reviews.llvm.org/D12239
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
