[PATCH] D14209: CodeView type info support preview (LLVM portion)

[Amjad Aboud via llvm-commits]

aaboud added a comment.

Hi Dave,
I understand that this patch is an initial one.
However, I have some concerns that might be related to the design more than to the code itself.

Please see below.

Thanks,
Amjad


================
Comment at: include/llvm/CodeView/TypeIndex.h:156
@@ +155,3 @@
+
+inline bool operator==(const TypeIndex& A, const TypeIndex& B)
+{
----------------
What would be the usage of these operators?
Is there a need to sort the TypeIndex in list?
Does it make sense to have an order between basic types, e.g. int vs. float?

================
Comment at: include/llvm/CodeView/TypeRecord.h:13
@@ +12,3 @@
+
+class TypeRecord
+{
----------------
It might be a good idea that all records: type, symbol, id, etc. be derived from a single base object.
For example: CodeViewRecord.

================
Comment at: include/llvm/CodeView/TypeRecord.h:49
@@ +48,3 @@
+public:
+  ProcedureRecord(TypeIndex ReturnType, CallingConvention CallConv,
+    FunctionOptions Options, uint16_t ParameterCount,
----------------
I do not see the connection between TypeIndex and TypeRecord, and I am not sure that it is a good idea that the TypeRecord derived classes requires TypeIndex class as a reference to other TypeRecord.

In my opinion, it would be better to:
1. have a TypeIndex member in TypeRecord.
2. change TypeRecord derived classes to require TypeRecord pointer instead of TypeIndex.

================
Comment at: lib/IR/DebugInfoMetadata.cpp:313
@@ -312,1 +312,3 @@
 
+DICodeViewTypes *DICodeViewTypes::getImpl(
+  LLVMContext &Context, unsigned Signature, Metadata *TypeRecords,
----------------
It have been already said, but I want to second it.
I do not think that it is a good idea to have CodeView debug info entries in the metadata.
I believe that the current DINodes are sufficient to create CodeView debug info.
And if there are few things that need to be extended, let's do it.


http://reviews.llvm.org/D14209