[PATCH] D26013: Change DWARF parser to use enumerations for DWARF tags, attributes and forms.

[David Blaikie via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h:46-47
+        return AttributeSpecs[idx].Form;
+    else
+        return dwarf::Form(0);
   }
----------------
Skip the else after return ( http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return )


================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:42-45
-    if (CurOffset == *OffsetPtr) {
+    const Attribute A = static_cast<Attribute>(Data.getULEB128(OffsetPtr));
+    const Form F = static_cast<Form>(Data.getULEB128(OffsetPtr));
+    if (A && F) {
+        AttributeSpecs.push_back(AttributeSpec(A, F));
+    } else if (A == 0 && F == 0) {
+      // Successfully parsed this abbreviation
+      break;
+    } else {
+      // Either the attribute or form was zero, but not both which is an error
       clear();
       return false;
     }
----------------
What happened to these error paths?


================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:44
   while (true) {
-    uint32_t CurOffset = *OffsetPtr;
-    uint16_t Attr = Data.getULEB128(OffsetPtr);
-    if (CurOffset == *OffsetPtr) {
+    const Attribute A = static_cast<Attribute>(Data.getULEB128(OffsetPtr));
+    const Form F = static_cast<Form>(Data.getULEB128(OffsetPtr));
----------------
Drop the const from local values - we don't generally do that (& can be confused with "const T&" - might be read as though a reference was intended & accidentally omitted)

Also, if you like, you can use "auto" for the type when the RHS has the type prominently in the static_cast type parameter anyway.

(as you did later with AtomForm)


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp:211
   for (const auto &AttrSpec : AbbrevDecl->attributes()) {
-    uint16_t Form = AttrSpec.Form;
+    const auto Form = AttrSpec.Form;
 
----------------
Drop the const


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp:237
+bool DWARFDebugInfoEntryMinimal::getAttributeValue(const DWARFUnit *U, 
+    const dwarf::Attribute Attr, DWARFFormValue &FormValue) const {
   if (!AbbrevDecl)
----------------
I'd probably drop the const here, even though it was there in the original.

(similarly in a few cases below)


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:126-127
     return (FC == FC_String);
+  default:
+    break;
   }
----------------
aprantl wrote:
> Here's an idea: How about adding a
> ```
> template<typename EnumType>
> llvm::Optional<EnumType>
> getULEB128AsEnum() {
>   // Decode ULEB.
>   uint64_t Val = ...; // what about larger values?
>   if (Val >= EnumType::getMinValue() && Val <= EnumType::getMaxValue())
>     return static_cast<EnumType>(Val);
>   return None;
> }
> ```
> ?
> 
> This way we could guarantee that on of the enums if it is passed as an enum is always well-formed. Then we would never need a default case in a switch over a dwarf enum and could benefit from the not-all-enum-cases-covered-by-switch warning.
> 
> Is that feasible?
What's this added/here for?


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:278-280
+    // Cast to uint16_t so we don't get a warning about all cases covered
+    // since we might dump newer DWARF someday that will have unsupported
+    // DW_FORM enumerations
----------------
Perhaps it'd work to have enums defined for the beginning of the user-defined extension space - and just never add support for those to these switches, then they wouldn't be fully covered switches and the warning wouldn't fire on the default?


Repository:
  rL LLVM

https://reviews.llvm.org/D26013