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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked 4 inline comments as done.
SouraVX added inline comments.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:852
+                    (Tag, Name, StringLength, StringLengthExp, SizeInBits,
+                     AlignInBits, Encoding))
+
----------------
aprantl wrote:
> SouraVX wrote:
> > aprantl wrote:
> > > Are these copy&paste errors?
> > Nope, just for backward compatilbility I guess ? just like other nodes. Notice one expects `MDString` and other one `StringRef`
> I see. But since this is a new node, we don't need all the variants with the default arguments, do we? I think I'd be more comfortable with the minimal set we can get away with.
This looks reasonable ?


================
Comment at: llvm/lib/IR/LLVMContextImpl.h:429
+  unsigned getHashValue() const {
+    return hash_combine(Tag, Name, SizeInBits, AlignInBits, Encoding);
+  }
----------------
aprantl wrote:
> 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.
Trimmed down to Tag + Name + Encoding.
Thanks for pointing this out!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86305