[PATCH] D86305: [DebugInfo]Added support for representing Fortran assumed length strings

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Thanks, this looks good to me — I just have a few inline comments.



================
Comment at: llvm/docs/SourceLevelDebugging.rst:1057
 
+There are a few DWARF TAGS defined to represent Fortran specific constructs i.e DW_TAG_string_type for representing Fortran character(n). In LLVM this is represented as DIStringType.
+
----------------
`DWARF tags`


================
Comment at: llvm/docs/SourceLevelDebugging.rst:1069
+
+and this will materialize in DWARF TAGS as:
+
----------------
same here


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:852
+                    (Tag, Name, StringLength, StringLengthExp, SizeInBits,
+                     AlignInBits, Encoding))
+
----------------
Are these copy&paste errors?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:787
+  if (STy->getEncoding()) {
+    // for eventual unicode support
+    addUInt(Buffer, dwarf::DW_AT_encoding, dwarf::DW_FORM_data1,
----------------
`// For eventual Unicode support.`


================
Comment at: llvm/lib/IR/LLVMContextImpl.h:429
+  unsigned getHashValue() const {
+    return hash_combine(Tag, Name, SizeInBits, AlignInBits, Encoding);
+  }
----------------
My guess is that we can get away with only hashing Tag+Name, or Tag+Size+Encoding, but I'll leave that up to you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86305/new/

https://reviews.llvm.org/D86305