[PATCH] D37157: Fix Bug 30978 by emitting cv file checksums.

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:476-481
+  enum class ChecksumKind : uint8_t {
     CSK_None,
     CSK_MD5,
     CSK_SHA1,
     CSK_Last = CSK_SHA1 // Should be last enumeration.
   };
----------------
ecbeckmann wrote:
> zturner wrote:
> > If you're going to make it an enum class, then it doesn't need the `CSK` prefix in the name, since you have to write `ChecksumKind` anyway.  I wouldn't suggest churning the code to change the names everywhere it's used, so instead maybe just leave this as a regular enum.  It's already scoped to `DIFile` anyway, so there's no risk of namespace pollution.
> Unfortunately I need the actual enum values to correspond exactly to the checksum types flag integer values actually present in the codeview section, hence the strongly typed enum class.  This is important because I will now be emitting the ChecksumKind as assembly directives as well as directly into the binary.
If you need the enums to have specific values, e.g. because those values end up in the object file (hard to tell here but I think that's what you are saying), you should set the enum values explicitly.  This documents the exact values you need (even if they happen to be sequential at the moment).  Making it an enum class doesn't get you that.


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:357
 
-static const char *ChecksumKindName[DIFile::CSK_Last + 1] = {
-  "CSK_None",
-  "CSK_MD5",
-  "CSK_SHA1"
-};
+static const char
+    *ChecksumKindName[static_cast<uint8_t>(DIFile::ChecksumKind::Last) + 1] = {
----------------
If you require exact correspondence between an enum and some array of strings, we usually do that with a .def file and macros.  See for example Dwarf.def. As it is, the association between this array and the ChecksumKind values is implied rather than explicit.  Safer to be explicit.


https://reviews.llvm.org/D37157